Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to disallow direct state modification from component #58

Open
Aaron-Pool opened this issue Jan 21, 2020 · 27 comments · May be fixed by #493
Open

Add an option to disallow direct state modification from component #58

Aaron-Pool opened this issue Jan 21, 2020 · 27 comments · May be fixed by #493
Assignees
Labels
discussion feature request plugins Related to existing plugins

Comments

@Aaron-Pool
Copy link

In the example, I kind of dislike the fact that the component can directly call cart.state.rawItems = [];. Just because I think that can encourage people to modify state in a disorganized manner. Can we get a plugin setting that disallows state being modified from the component (rather than an explicit action)?

@Aaron-Pool Aaron-Pool changed the title Add an option to disallow direct state modification Add an option to disallow direct state modification from component Jan 21, 2020
@posva
Copy link
Member

posva commented Jan 22, 2020

I think disallowing direct state modification is a rule that should be enforced at a linter level instead because runtime-wise this would only be a dev-only warning, so it would be slower during dev and require more code in the library

Being able to directly modify the state (or using patch) is intentional to lower the entry barrier and scale down. After many years using Vuex, most mutations were completely unnecessary as they were merely doing a single operation via an assignment (=) or collection methods like push. They were always perceived as verbose, no matter the size of the project, adding to the final bundle size as well, and were useful only when grouping multiple modifications and some logic, usually in larger projects. In Pinia, you are still able to have these groups by using actions but you get to choose if you need them or not, being able to start simple, and scale up when necessary.

So I want to make people realise mutations is an unnecessary concept thanks to actions and that the cost of mutations and its verbosity is not enough to justify the possibility of organisation it brings. On top of that, one of the main aspects of mutations was devtools: being able to go back and forward. In Pinia, all modifications are still tracked so doing direct state modification still allows you to keep things organised despite it wasn't taught that way with Vuex

@Aaron-Pool
Copy link
Author

@posva just to clarify, I wasn't requesting to disallow direct state mutation in actions. I was only requesting to disallow direct state mutation from component methods. I agree that mutations feel like overkill, and I'm happy to see the concept of mutations is absent from pinia. I do, however, think that only being allowed to modify state through actions encourages a concise, and thought-through API for state management and modification. It also creates an incentive to only put state in vuex that really needs to be there, and use component local state for everything else, rather than the "put everything in vuex" approach that I often see people take, which doesn't seem to scale well.

All that being said, your reasoning about it being a linter-level feature, rather than an implementation feature makes perfect sense 👌

Thanks for the quick response!

@posva
Copy link
Member

posva commented Jan 22, 2020

I wasn't requesting to disallow direct state mutation in actions

Yes, of course, disallowing it in actions would make my response nonsense 😆

@sisou
Copy link

sisou commented Feb 1, 2020

Vue 3 includes a readonly() method that makes anything passed into it read-only. This could be used for the exported state, which would show an error with Typescript and log an error/stop mutations during runtime.

Futhermore, Typescript has a Readonly<> type modifier that can be used for Typescript errors already in Vue 2.

Would you consider using either of those?

@raphaelkieling
Copy link

And the patch still exist? because if i would need show or hide a card, i will need create a action called showCard(active) it's sounds a vuex mutation. I agree with @posva to only create a dev warning, this is horrible to scale larger pages but in some cases this is a option

@koljada
Copy link

koljada commented Sep 27, 2020

I am also trying to avoid mutating the state outside the store and would love to use actions for that.
How is it possible to show linter warnings when changing the store's state in components?

@posva posva added the plugins Related to existing plugins label Apr 9, 2021
posva added a commit that referenced this issue May 12, 2021
@posva posva linked a pull request May 12, 2021 that will close this issue
@posva
Copy link
Member

posva commented May 12, 2021

Opened #493 to try to get more feedback

@soylomass
Copy link

soylomass commented Jul 26, 2021

In a little store experiment I was working on before finding about Pinia, thinking about this problem, I implemented the following solution:

Add an "autoSetters" boolean that would to the configuration:

  • Make the exposed state readonly
  • For each state property, automatically create a setter function (store.name -> store.setName). Used TS template literals to make typescript and code completion work as expected.

So for the following state:

export const useMainStore = defineStore({
  id: 'main',
  autoSetters: true,
  state: () => ({
    counter: 0,
    name: 'Eduardo',
  })
});

You would get:

const main = useMainStore();
// Error
main.counter = 2;
// Ok
main.setCounter(2);
// Error
main.name = 'Fede';
// Ok
main.setName('Fede');

I found this to be natural and worked fine. Is there any issue or caveat you can think of about this kind of solution?

I don't really like the fact of having "magic" methods auto-generate, but it was the best-looking / less verbose of all solutions I could think of.


EDIT: Now that I re-read the original comment, I realized that the point of the issue was to centralize state modification inside explicitly defined actions, so this wouldn't solve that. My goal was just to make state modification an explicit action, instead of just a variable modification.

@seangwright
Copy link

This seems like something worth enforcing but also only via linting, since it's kind of a stylistic concern.

I completely agree that store state should not be mutated by components directly. In large apps it becomes very difficult to tell where state changes are being initiated from.

Forcing the change to happen in an action means that property assignment becomes method calls and method calls. I find that with a method call, it's reasonable to expect a side-effect, whereas property assignment initiated side-effects can be difficult to predict.

I do think that mutations (in the Vuex sense) are not helpful, even in large apps, but I do think that as an app grows in size, forcing state changes to go through specific centralized calls in a store can help make stores more predictable.

I've also had situations where I wanted state to be mutable (so a getter wouldn't be applicable), but only from within the store.

...

I guess I'm not sure how much Pinia should enforce stylistic things or best practices, especially when comparing it to the boilerplate and architecture required for Vuex. Pinia has a better grow-up story at the moment.

@miaulightouch
Copy link

miaulightouch commented Sep 7, 2021

it seems we can define store via composition api in pinia 2.0?

I use pinia with readonly from typescript to expose state like

import { computed, reactive } from 'vue';
import { defineStore } from 'pinia';

type DeepReadonly<T> = { readonly [P in keyof T]: DeepReadonly<T[P]> }; // @microsoft/TypeScript/#13923
type EnvType = 'prod' | 'test';

export const useMainStore = defineStore('main', () => {
  const state = reactive({
    envType: 'prod' as EnvType,
    nestedObj: { something: 'good' },
  });

  // Actions
  const setEnvType = (payload: EnvType) => { state.envType = payload; };

  return {
    ...computed<DeepReadonly<typeof state>>(() => state), // spread as store properties
    state: computed<DeepReadonly<typeof state>>(() => state), // export entire state
    setEnvType,
  }
});

const store = useMainStore();

// you cannot assign value due to ts(2540): read-only property
store.state.envType = 'test';
store.envType = 'test';
store.state.nestedObj.something = 'bad';
store.nestedObj.something = 'bad';

it may not block state access in runtime, but it would work when linting or compiling.

@bitx1
Copy link

bitx1 commented Dec 1, 2021

it may not block state access in runtime, but it would work when linting or compiling.

Vue 3.2.23 fixes the reactivity bug where readonly proxies were not being preserved. With the fix, you can no longer update a store readonly value in runtime which is expected behavior.

It is now possible to disable state modification using composition setup API. Simplified example...

export const useAppStore = defineStore('app', () => {
  const state = reactive({
    counter: 0
  });

  // actions
  function increment() {
     state.counter++;
  }

  return {
    state: readonly(state),
    increment
  }
});

@jmroon
Copy link

jmroon commented Dec 20, 2021

I think you should be able to keep the composition API store structure consistent with the options API by doing the following:

export const useAppStore = defineStore('app', () => {
  const state = reactive({
    counter: 0
  });

  const actions = {
    increment() {
     state.counter++;
    }
    // other actions can go here
  }

  return {
    ...toRefs(readonly(state)),
    ...actions
  }
});

@posva

This comment has been minimized.

@jmroon
Copy link

jmroon commented Jan 19, 2022

I did run into some issues when it came to writing a store using composition.

I understand that a lint rule is one way to solve this, but would it just be possible to add a config option to the store to make the state returned as readonly(state)? Seems this is all that's needed with Vue 3 to make the state readonly, and it would be a mighty fine feature to be able to do this without having to resort to composing the store manually. It also seems preferable to VueX's strict option as it would provide compile time validation rather than runtime. As this is such a common pattern with application stores, it does feel like it would be worth supporting out of the box rather than relying on a 3rd party linter.

@ealldy
Copy link

ealldy commented Feb 25, 2022

One more thing to keep in mind:

If you use computed to return values from mapStores, you need to set the return type to DeepReadonly<state property type> for it to work, otherwise it'll complain about this.myStore being unknown. Not sure why that is.

I opened a new proposal in vuejs/core seems relevant to this problem.
unReadonly proposal

@carbotaniuman
Copy link

I'd love a way to enforce that all mutations must be performed in the context of an action (or with some escape hatch that can be linted against), simply because the network synchronization we use hook into actions, and it's very error prone to have a direct state mutation that does nothing.

@8bitDesigner
Copy link

I would love this feature personally; being able to control setter access is big reason why I’m on Vuex still

@Inori-Lover
Copy link

is it mean i can no longer write below code?

<input v-model="store.inputVal" />

@dakt
Copy link

dakt commented Oct 17, 2022

Honestly, mutating state directly from outside of store just doesn't make any sense. I can't see any benefits to be honest. We just had huge bug thanks to this "feature".

@seahindeniz
Copy link

@dakt I have a use case where I only have to listen only mutations and changing it directly is easier rather than actions. Because you actually don't have to know where you mutated, only the mutation matters.
But of course, to me this issue also makes sense, but not for all projects/cases.

We can simply say it depends

@andrewg-camus
Copy link

If any TypeScript users come across this, I created an alternative defineStore that returns a store definition with a readonly State type. It at least prevents casual mutation of state from type-checking. There's no runtime enforcement.

/**
 * A replacement for `defineStore` which makes all state properties readonly
 * to prevent mutations outside of actions.
 */
export function defineImmutableStore<Id extends string, S extends StateTree = {}, G extends _GettersTree<S> = {}, A = {}>(
  id: Id,
  options: Omit<DefineStoreOptions<Id, S, G, A>, 'id'>
): StoreDefinition<Id, Readonly<S>, G, A> {
  return defineStore(id, options)
}

@qiaokeli111
Copy link

In the example, I kind of dislike the fact that the component can directly call cart.state.rawItems = [];. Just because I think that can encourage people to modify state in a disorganized manner. Can we get a plugin setting that disallows state being modified from the component (rather than an explicit action)?

i write a plugin that you don't need change any code , just use my plugin, yuu can try it. my plugin address is https://www.npmjs.com/package/pinia-plugin-readonlystate

@jmroon
Copy link

jmroon commented Apr 18, 2023

Building on @andrewg-camus's solution, I have augmented pinia app wide to support this behavior seemlessly.

Create a pinia-readonly-state.d.ts (or whatever name you prefer) in the src directory. We can use module augmentation to override the pinia return types:

import {
  _ExtractActionsFromSetupStore,
  _ExtractGettersFromSetupStore,
  _ExtractStateFromSetupStore,
  _GettersTree,
  DefineSetupStoreOptions,
  DefineStoreOptions,
  StateTree,
  StoreDefinition,
} from 'pinia'

import 'pinia'
import { DeepReadonly } from 'vue'

declare module 'pinia' {
  export declare function defineStore<
    Id extends string,
    S extends StateTree = {},
    G extends _GettersTree<S> = {},
    A = {}
  >(
    id: Id,
    options: Omit<DefineStoreOptions<Id, S, G, A>, 'id'>
  ): StoreDefinition<Id, DeepReadonly<S>, G, A>

  export declare function defineStore<
    Id extends string,
    S extends StateTree = {},
    G extends _GettersTree<S> = {},
    A = {}
  >(options: DefineStoreOptions<Id, S, G, A>): StoreDefinition<Id, DeepReadonly<S>, G, A>

  export declare function defineStore<Id extends string, SS>(
    id: Id,
    storeSetup: () => SS,
    options?: DefineSetupStoreOptions<
      Id,
      DeepReadonly<_ExtractStateFromSetupStore<SS>>,
      _ExtractGettersFromSetupStore<SS>,
      _ExtractActionsFromSetupStore<SS>
    >
  ): StoreDefinition<
    Id,
    DeepReadonly<_ExtractStateFromSetupStore<SS>>,
    _ExtractGettersFromSetupStore<SS>,
    _ExtractActionsFromSetupStore<SS>
  >
}

Note that we need to use DeepReadonly to ensure that deeply nested objects and arrays are not mutable. This will also prevent the usage of array mutation methods (i.e. push).

Note there is still one unsolved problem: getters return types are still mutable. Haven't quite cracked how to address that.

Also I can see @posva's point as to why this should probably be a linting rule. Readonly/deepreadonly objects from the store won't work as typical props into components unless those props are also wrapped in DeepReadonly. A linting rule similar to https://eslint.vuejs.org/rules/no-mutating-props.html would be ideal.

@volarname
Copy link

any update on this?
what about to create function similar to storeToRefs for example storeToReadonly that will return readonly version with working reactivity
then you can just do following:

const someStore = useSomeStore()
const { state1, state2, getter1, getter2 } = storeToReadonly(assetListStore) // for state refs and getters
const { action1, action2 } = someStore // for actions
// this will work
action1()
action2()
const newValue = state1.value + 5
const newValue = getter1.value + 5 // as getters are computed
// this will show error: TS2540: Cannot assign to 'value' because it is a read-only property.
state1.value = 10

@tassin-gauthier
Copy link

tassin-gauthier commented Aug 18, 2023

is it possible to disallow direct state modification from component using Vue 3 but with Option API ? The readonly function is specified to Composition API.

I also tried something like this :

export const myStore = defineStore(
  "my-store",
  {
    state: () => ({
      key: "value"
    } as const),
    actions: {
       // my actions...
    },
  }
)

with the as const, but of course if I do this, I can't update my state anymore, even in actions.

@JayPuff
Copy link

JayPuff commented Feb 19, 2024

is it possible to disallow direct state modification from component using Vue 3 but with Option API ? The readonly function is specified to Composition API.

I also tried something like this :

export const myStore = defineStore(
  "my-store",
  {
    state: () => ({
      key: "value"
    } as const),
    actions: {
       // my actions...
    },
  }
)

with the as const, but of course if I do this, I can't update my state anymore, even in actions.

For me personally, I used the method another person posted above. Define the store using the composition style syntax that gives more flexibility, and on your return object, return the state as readonly return { state: readonly(state) }

This prevents me from modifying the state directly on components whether it's composition API defined components, or options API components. (I'm using the store in options API components, by injecting it using setup(), and then using it on the component through this.someStore)

@zunnzunn
Copy link

zunnzunn commented Apr 12, 2024

I also think this should be something enforced by a linter rule. The solution with vue's readonly works, but has drawbacks:

  1. It slightly changes the type of the state, which might have drawbacks for TypeScript users in some cases.
  2. In unit tests, it makes sense to mutate the store state directly for the sake of creating certain test constellations.

I am looking forward to that eslint plugin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion feature request plugins Related to existing plugins
Projects
None yet
Development

Successfully merging a pull request may close this issue.