-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Comments
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 So I want to make people realise |
@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! |
Yes, of course, disallowing it in actions would make my response nonsense 😆 |
Vue 3 includes a Futhermore, Typescript has a Would you consider using either of those? |
And the |
I am also trying to avoid mutating the state outside the store and would love to use actions for that. |
Opened #493 to try to get more feedback |
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:
You would get:
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. |
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. |
it seems we can define store via composition api in pinia 2.0? I use pinia with 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. |
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
}
}); |
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
}
}); |
This comment has been minimized.
This comment has been minimized.
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. |
I opened a new proposal in vuejs/core seems relevant to this problem. |
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. |
I would love this feature personally; being able to control setter access is big reason why I’m on Vuex still |
is it mean i can no longer write below code?
|
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". |
@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. We can simply say it depends |
If any TypeScript users come across this, I created an alternative
|
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 |
Building on @andrewg-camus's solution, I have augmented pinia app wide to support this behavior seemlessly. Create a 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 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. |
any update on this? 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 |
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 |
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 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 |
I also think this should be something enforced by a linter rule. The solution with vue's
I am looking forward to that eslint plugin ! |
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); |
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)?The text was updated successfully, but these errors were encountered: