vuejs / pinia

šŸ Intuitive, type safe, light and flexible Store for Vue using the composition api with DevTools support
https://pinia.vuejs.org
MIT License
13.18k stars 1.06k forks source link

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

Open Aaron-Pool opened 4 years ago

Aaron-Pool commented 4 years ago

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)?

posva commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

Yes, of course, disallowing it in actions would make my response nonsense šŸ˜†

sisou commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

Opened https://github.com/posva/pinia/pull/493 to try to get more feedback

soylomass commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@jmroon use a new discussion for that question, please

jmroon commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

Inori-Lover commented 2 years ago

is it mean i can no longer write below code?

<input v-model="store.inputVal" />
dakt commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 9 months ago

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 commented 7 months ago

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 !

stephane303 commented 3 months ago

Your thoughts on this solution, any drawbacks ?

import { defineStore } from "pinia";
import { reactive, computed, ComputedRef } from "vue";

export const useReadOnlyStore = defineStore("readonlyStore", () => {
    const state = reactive({
        count: 0,
        message: "Hello, Pinia!"
    });

    const getters = {
        doubleCount: computed(() => state.count * 2),
        uppercaseMessage: computed(() => state.message.toUpperCase())
    };

    const actions = {
        increment() {
            state.count++;
        },
        setCount(newCount: number) {
            state.count = newCount;
        },
        setMessage(newMessage: string) {
            state.message = newMessage;
        }
    };

    type State = typeof state;
    const readonlyState = Object.fromEntries(Object.keys(state).map((key) => [key, computed(() => state[key as keyof State])])) as {
        [K in keyof State]: ComputedRef<State[K]>;
    };

    return {
        ...readonlyState,
        ...getters,
        ...actions
    };
});

Usage:

const readOnlyStore = useReadOnlyStore();

readOnlyStore.count = 3; // Vue: Cannot assign to 'count' because it is a read-only property.
readOnlyStore.setCount(3);