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

fix(State): Return state as a function #4

Closed
wants to merge 1 commit into from
Closed

fix(State): Return state as a function #4

wants to merge 1 commit into from

Conversation

mrtnvh
Copy link

@mrtnvh mrtnvh commented May 27, 2019

If state is a function, it is already called in this library, when in fact the function should be called in Vuex itself.

I couldn't create a test to test this behaviour, but Typescript currently states that buildState returns a type of <any>, whereas the build_config.ts states that state should return a Function. This PR fixes that behaviour.

This change should be implemented because of the thorough explanation found here: https://stackoverflow.com/questions/49557177/vuex-state-returned-as-function-or-object-literal

I'm happy to hear your thoughts.

@mrtnvh
Copy link
Author

mrtnvh commented Jan 7, 2020

Hi @eunjae-lee,

Could I get an update on this?
It would be awesome to use this package in my Nuxt projects without any warnings.

Tnx.

@eunjae-lee
Copy link
Owner

Hi @vanhoofmaarten !
I'm sorry. For whatever reason, I wasn't watching my own repo 🤦‍♂ .
I haven't received any notification.
I will get back to you as soon as I grab some time to digest this.

@mrtnvh
Copy link
Author

mrtnvh commented Jan 7, 2020

That explains. 😅
Good luck. Happy to hear from you soon!

@eunjae-lee
Copy link
Owner

Hi @vanhoofmaarten,

Typescript currently states that buildState returns a type of
→ correct.

whereas the build_config.ts states that state should return a Function.
→ incorrect.

image

`build_config.ts` says `state` IS a Function (without explicit return type). So, back in `state_builder.ts`,

image

buildConfig.state() is a valid function call, and since build() has no explicit return type, it can return buildConfig.state() without any problem.

So, I'm not sure if I understand your problem.
Can you show me the warnings you see?

@mrtnvh
Copy link
Author

mrtnvh commented Jan 11, 2020

I indeed need to provide some extra clarification, as my initial proposal did not explain the issue I was having.

The reason I made this PR is because I use this package with Nuxt.js. When you use a store in Nuxt, it's default implementation is build around modules. Nuxt requires you to define your state property as a function,, because they use the Model Reuse pattern described by the Vuex documentation.

I've created a CodeSandbox with vuex-dry used within a Nuxt project.
https://codesandbox.io/s/github/vanhoofmaarten/vuex-dry-pr-1/tree/master/?expanddevtools=1&fontsize=14&hidenavigation=1&theme=dark

When you look at the console, you see that Nuxt throws warnings regarding not declaring the state as a function.

That's the main thing I want to fix in this PR.

Instead of executing the vuex-dry state function and returning the object within,

function build(buildConfig: ModuleBuildConfig) {
return buildConfig.state();
}

I propose just to return the function to Vuex. This fixes the warning and enables Model Reuse.

function build(buildConfig: ModuleBuildConfig) {
return buildConfig.state;
}

I hope everything is more clear now.

Thanks!

@eunjae-lee
Copy link
Owner

Thanks for the thorough explanation. Since your change brings a breaking change, I took a little different approach.

https://codesandbox.io/s/serverless-cdn-1i2pk

I hope this works! (0.2.1)

export default Module.build({
  stateAsFunction: true,
  state: () => ({
    list: []
  }),
  ...
});

@mrtnvh
Copy link
Author

mrtnvh commented Jan 28, 2020

Perfect! Thank you!

@mrtnvh mrtnvh closed this Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants