feat: allow strict option

[posva]

Close #58

This still needs work, it's more of an experiment yet. Feel free to pick it up and move forward or propose API ideas

So far:

• Make store.aStateProperty readonly
• Allow changes in actions
• Allow changes to store.$state
• Allow calls to store.$patch() anywhere

A runtime check could be added in dev mode only as well

[codecov-commenter]

Codecov Report

Merging #493 (5745ce8) into v2 (e8f4b0e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##                v2      #493   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          220       220           
  Branches        38        38           
=========================================
  Hits           220       220           

Impacted Files	Coverage Δ
src/rootStore.ts	100.00% <ø> (ø)
src/types.ts	100.00% <ø> (ø)
src/store.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8f4b0e...5745ce8. Read the comment docs.

[Aaron-Pool]

As the author of the original issue, just figured I'd drop in and say that this PR checks all the boxes for me. The biggest thing for me was to encourage intentional, concise, and deliberate state modification. Rather than having state modification be scattered all over the place. Makes state management easier to reason about, in my opinion.

So, all that to say, thumbs up from me.

[posva]

Thanks for the feedback @Aaron-Pool !

[7ammer]

I like this feature! 👍

[vladcosorg]

Just moved out team to Pinia and that was the first (and only) request from my team members. Perfect state manager for vue, goodbye vuex verbosity, thanks @posva !

[posva]

I still believe an eslint plugin would be more flexible and configurable. It would also benefit JS users instead of only being usable with TypeScript.

Unfortunately, I don't have that much experience with eslint yet but I have plans into creating @pinia/eslint-plugin. Only then, I will be able to compare both methods but an eslint plugin is more likely to help big codebases and teams

[lijialiang]

I also like this future, is there a version planned to merge?

[cbeninati]

I'd love to see this merged. Is there anything I can help with to move it forward?

[MariusHendriks]

I'd like this PR to be merged as well 😄

[stephane303]

Why is this not merged ? This is quite critical, as for now using Vue2, there are some issue with DevTools, the timeline is not showing the access to the store any more, at least is my case.
Without this feature the store is like a giant global variable that we don't know who and when it's accessed. This makes debugging very difficult, even impossible.

Any suggestion for a workaround ?

The only workaround "working" for me is this:

export const useSettingsStore = defineStore("settings", () => {
    const state = reactive({
        loading: true,
        initialisationError: "",
        initialized: false,
        regionModeEnabled: true,
       ......
    });
    return {
        ...computed<DeepReadonly<typeof state>>(() => state),   // this make typescript signal issues when some try to write directly in the state
        ...toRefs(state)                                        // this gives me access to my state values
    };
});

I am trying to figure out how to use only one element in the return, but so far without luck, I either loose the typescripts errors, or the content of the state.

[MariusHendriks]

@stephane303 I had the same problem with the returning part that it needed to be readonly. Instead of returning the state as a readonly you could fetch the state through mapGetters(). This makes them readonly as well (in the component) and i think they will still appear in the development console. Problem is; all developers in your project need to use it that way, otherwise it can turn into a real mess real quick.

[stephane303]

@MariusHendriks thanks, but we already have a large codebase and I dont' want to change everything, but strangely this works, I know it looks like cheating but it warns me and get my values. Really need to move to Vue 3 soon ...

export const useSettingsStore = defineStore("settings", () => {
    const state = reactive({
        loading: true,
        initialisationError: "",
        initialized: false,
        regionModeEnabled: true,
       ......
    });
    return {
        ...toRefs<ComputedRef<DeepReadonly<typeof state>>>(state as any),
    };
});

[stephane303]

Finally I did this:

import { useSettingsStoreProtected } from "@/cockpit/stores/settingsStoreProtected";
import { Pinia } from "pinia";
import { cloneDeep } from "lodash"
export function useSettingsStore(pinia?: Pinia) {
    const settingsStoreProtected = useSettingsStoreProtected(pinia);

    const handler = {
        set(target: any, prop: any) {
            console.error("Do not access settings state directly, the property used is:", prop);
            // eslint-disable-next-line prefer-rest-params
            return Reflect.set(...arguments);
        },
        get(target: any, prop: string | symbol) {
            // eslint-disable-next-line prefer-rest-params
            const getObject = Reflect.get(...arguments);
            if (typeof getObject === "function") {
                return getObject;
            } else {
                if (prop != "__ob__" && prop in settingsStoreProtected.$state) {
                    //console.log("cloned ", settingsStoreProtected.$state, prop);
                    const clone = cloneDeep(getObject);
                    return clone;
                }

                return getObject;
            }
        }
    };
    const proxy: typeof settingsStoreProtected = new Proxy(settingsStoreProtected, handler);
    return proxy;
}

To be sure the state is not accessed accidentally.

[prodoxx]

I still think having this option would be a great addition.

[prodoxx]

They will not merge this because they are not working on pinia anymore i guess ^^

As much as I agree that there needs to be some flexibility in some packages, it's sometimes important for enforce some best practices. For now, I'm doing:

interface StoreState {
    aStateVar: string;
}

interface StoreReturnValues {
    state: DeepReadonly<StoreState>;
}

const useStore = defineStore('someStore', (): StoreReturnValues => { 
    ....
    return { state }
})

and this shows a warning message in vscode when any state property is attempted to be directly changed but it doesn't stop the app from running. It's fine for now but someday someone in my team will forget to make state DeepReadonly and directly change the state in a component without getting a warning message.

[reinzor]

@posva , could you give an update about the plan of merging this PR?

[AlmarAubel]

any update on this? It would be really nice to have this function available.

[lorand-horvath]

@posva Why is this not being merged? A strict option to disallow store state mutations directly from components is needed to discourage bad practices.
Or the dev should know better?

[guska8]

This is such an important change! We have a huge codebase using vue2/vuex that will be ported to vue3/pinia soon and the code would be much safe with this option enabled.

[dpschen]

Maybe a workaround worth noting: You can wrap exported state with readonly(). E.g.

export const useMyStore = defineStore("myStore", () => {
	const myValue = ref(true)
	
	function setMyValue(newMyValue: boolean) {
		myValue.value = newMyValue
	}
	
	const myState = reactive({ someProperty: 'value' })
	
	return {
		myValue: readonly(myValue),
		myState: readonly(myState),
	
		setMyValue,
	}
})

[volarname]

what about this? #58 (comment)

[Renatopster]

@posva any chance you can share an update about this? Feedback seemed very positive, so maybe we can help you solve any concerns that you might still have in order to get it merged 🥺

[BenJackGill]

+1 from me also! Would love to see this :)