davestewart / vuex-pathify

Vue / Vuex plugin providing a unified path syntax to Vuex stores
https://davestewart.github.io/vuex-pathify
MIT License
1.37k stars 57 forks source link

Unable to use with tests due to caching of getters #70

Closed axwalker closed 5 years ago

axwalker commented 5 years ago

We have a suite of tests for our various components, some of which use vuex, some of which use vuex and vuex-pathify (as we migrate non-pathified stuff over after discovering your library).

However, we have a problem when running several different tests against a component that uses vuex-pathify. It's not possible to set different initial vuex state for each test. Any get usage in the component always gets the data from the store that was used in the first test, rather than the newer stores used in subsequent tests.

In case this isn't clear, I'll try and make a minimal example on codesandbox to demonstrate when I get chance tomorrow.

Having drilled into the problem, I can see the issue is caused by this logic. The getter is essentially cached the first time it's used, which means it's always pointing to the store from the first test.

Is this related to the cache option that is marked as not yet implemented? I tried changing the above function to instead include:

    if (!options.cache || !getter) {
      getter = makeGetter(this.$store, path, stateOnly)
    }

and was then able to get the behaviour I wanted by setting cache to be true just for our tests. Is this what you had in mind with the caching option? Or was that related to something else?

axwalker commented 5 years ago

Here is a minimal codesandbox example demonstrating the issue:

https://codesandbox.io/s/holy-fast-hqkbu?fontsize=14&module=%2Fsrc%2Fsample.test.js&previewwindow=tests

davestewart commented 5 years ago

Hey Andrew,

Thanks for the clearly-written issue and example!

Hmmm. Good catch. I don't do a hell of a lot of testing (wrist slap!) so I'm glad you caught this one.

The cache option never made it in, and I can't remember why I thought it was important right now. I think it had something to do with caching the generated helper functions in a hash so when they were asked for again (i.e. customers/current@xxx) it would just return the function rather than going to the effort of generating it again.

Back to your issue, and again, thanks for the example. This makes it really clear why it's not working - the generated getter function is a closure which already has a reference to the initial store, but you change that store when you generate the second test.

I can think of a few fixes here:

  1. Vuex pathify should generate a new getter function each time; but that would mean the getters would not be optimised, and would run through all the creation machinery every time they were used

  2. move the state reference into a function so it is not cached; but I don't know if this would affect the code elsewhere in the lib

  3. add a new option to disable the caching of the generated getter function from makeGetter()

  4. cache a reference to this.$store in getOne() and always do a quick comparison to this.$store when getting. This would effectively automate point 3 and mean I don't need to update the code in the rest of the app:

export function getOne (path, stateOnly) {
  let getter, store
  return function (...args) {
    if (!this.$store) {
      throw new Error('[Vuex Pathify] Unexpected condition: this.$store is undefined.\n\nThis is a known edge case with some setups and will cause future lookups to fail')
    }
    if (!getter || store !== this.$store) {
      store = this.$store
      getter = makeGetter(store, path, stateOnly)
    }
    return getter(...args)
  }
}

What do you think?

axwalker commented 5 years ago

Thanks a lot for the quick response!

I think option (3) or (4) sound sensible. Like you say, perhap option (3) is the simplest for now requiring the fewest changes. Do you think it would be fine to apply the same to the closures used in setOne and syncOne also?

I may be able to contribute some tests for vuex-pathify at some point if there's any interest?

davestewart commented 5 years ago

I think option (3) or (4) sound sensible.

I'll probably just go for 4 if it works, then no need to update the docs.

Do you think it would be fine to apply the same to the closures used in setOne and syncOne also?

Yeah, I would add it to all generators

I may be able to contribute some tests for vuex-pathify at some point if there's any interest?

That would be awesome! There's actually a ticket for that here:

https://github.com/davestewart/vuex-pathify/issues/4

Let me know if you need any help.

I may be able to get a fix out this week.

axwalker commented 5 years ago

Sorry, I meant option 4 as the simplest!

Great, I'll have a look at that ticket.

davestewart commented 5 years ago

OK - all published on NPM as 1.2.5 :)