keajs / kea-localstorage

Persist the state of your reducers in localstorage
https://kea.js.org/plugins/localstorage
MIT License
9 stars 3 forks source link

feat(Path): add ability to set prefix to localStore key #4

Closed sanchezweezer closed 5 years ago

sanchezweezer commented 5 years ago

This can be useful then you have two SPA on one domain =)

mariusandra commented 5 years ago

Hi, this is a nice idea! :)

However there are some things needed before I can merge the PR.

  1. This invalidates the API of the plugin from before... and I would like to avoid breaking changes when possible. Now you can't use it as plugins: [localStoragePlugin], but must use plugins: [localStoragePlugin()] when activating it. Since there's a way around this (something like let plugin = () => ({...}); Object.assign(plugin, plugin()); export default plugin;), let's work around it, so that the default export still acts as a regular object.

  2. I would prefer that the "plugin creator" get an options object (({ prefix })) as an input, not a single option ((prefix)), as there might be more options in the future and we should avoid another possible breaking change :).

  3. This needs a test to make sure that the prefix works.

  4. The documentation and the changelog need to be updated to reflect the change.

Regarding all of this, point 1 inspired me to add this change to Kea, so that in the future it would execute the function automatically when a plugin is exported as a function. This code would eliminate the need for point 1, but I would prefer not to backport it to the 0.28 series and spend all my energy getting 1.0 ready instead.

The 1.0 rc is very usable already, I'm just stuck behind cleaning up all the documentation for the launch :). Won't be too long now...

sanchezweezer commented 5 years ago

Hi, thanks =)

But really good idea - it's KEA. Use it on all my react-redux projects. And thanks for so quick answer.

Yes, I understand, i'm started merge discussion just to sure, is this feature needed or not.

In few days I do all this things, and try add it on new version. But I have questions about tests, because in 0.2.0 I don't see tests...

Yea, I try new version this hooks and some legacy code. it works really well. And right now I'm in migration process from 0.28 to 1.0.0, so I can add in both versions.

Documentation not a problem for use new version of Kea ^^ (IMHO), my team thinks the same.

Hi, this is a nice idea! :) However there are some things needed before I can merge the PR.

  • This invalidates the API of the plugin from before... and I would like to avoid breaking changes when possible. Now you can't use it as plugins: [localStoragePlugin], but must use plugins: [localStoragePlugin()] when activating it. Since there's a way around this (something like let plugin = () => ({...}); Object.assign(plugin, plugin()); export default plugin;), let's work around it, so that the default export still acts as a regular object.

  • I would prefer that the "plugin creator" get an options object (({ prefix })) as an input, not a single option ((prefix)), as there might be more options in the future and we should avoid another possible breaking change :).

  • This needs a test to make sure that the prefix works.

  • The documentation and the changelog need to be updated to reflect the change. Regarding all of this, point 1 inspired me to add this change to Kea, so that in the future it would execute the function automatically when a plugin is exported as a function. This code would eliminate the need for point 1, but I would prefer not to backport it to the 0.28 series and spend all my energy getting 1.0 ready instead. The 1.0 rc is very usable already, I'm just stuck behind cleaning up all the documentation for the launch :). Won't be too long now...

mariusandra commented 5 years ago

Hi, great and thank you!

Indeed, I added the tests in the "big-rewrite" branch. That branch will become master when kea 1.0 is out.

Regarding the kea documentation, it might not be issue for existing users (except if something goes wrong with the migration as there are some breaking changes), but at the very least the installation instructions and other guides need to be updated, so that new users wouldn't be given false information and left with a really frustrating experience :).

If you and your team want to and can help out, then the new docs are here... or live here: https://kea.netlify.com/ . The "why" and "what" parts of the homepage need to be rewritten (the "how does it work" part is done) and everything under "docs" needs to be reviewed and updated. Parts of it can probably just be deleted.

In any case, thanks again for this PR and the idea behind it. Keep an eye out for the updated docs and an eventual 1.0 stable very soon :).

sanchezweezer commented 5 years ago

So, do i need to transfer test to 0.2.0, or not? =)

About docs, this is only my initiative, in my free time, I'll try to help =)

mariusandra commented 5 years ago

Hey, of course no tests needed for the 0.2 version :). You can skip updating this version altogether if it's not needed for you... and directly make the changes in the 1.0 (big-rewrite) branch.

Thanks for the contribution! :)

sanchezweezer commented 5 years ago

@mariusandra Hi, again. I add some more to this pull request. Can you check it? Also, I prepare this params for new version, may be tomorrow add new request in other branch

sanchezweezer commented 5 years ago

test it on my real project with kea on board, so can say it should work fine)

sanchezweezer commented 5 years ago

I change CHANGELOG file. But don't know set the 0.2.1 or 0.3.0? Do I need to bump version?

mariusandra commented 5 years ago

Hi, few comments :)

  1. It appears you added semicolons to the end of every line, effectively marking every line as changed. Could you remove them and have the minimal changes necessary for this feature?

  2. The options for the reducer are slightly confusing: { persist: 'example' }. I would keep this as persist: true and add separator and prefix as additional options you can pass, which would then override the globals.

  3. For the version, this is at least a change to 0.3.0.

  4. I like the idea that you just import the plugin from 'kea-localstorage' and then if you want to add options, run plugins: [localStorage(options)], otherwise just do plugins: [localStorage]. With kea 1.0, all plugins that are exported as functions are executed automatically, however for now, instead of exporting a separate configure function, you can do something like this and the plugin can be used with or without options:

const plugin = (options) => ({ ... })
Object.assign(plugin, plugin())
export default plugin;
sanchezweezer commented 5 years ago

Hi, @mariusandra =)

Yes, version I set to 0.3.0, and I think set option separate is good idea, I just add them at the end, so don't think about them well.

But about №4: I really like this option, but this is hard to do, because of this: https://github.com/keajs/kea/blob/fe74415783981bdcaf7d758a869710296ab8520e/src/kea/plugins/index.js#L51

sanchezweezer commented 5 years ago

Otherwise, I can create pull request to main kea pack =)))

mariusandra commented 5 years ago

Thank you! Version 0.3.0 is out with the change! :)

However since I just now also released Kea 1.0, I will merge this change into the 1.0 plugin branch and release the 1.0 version of this plugin as well.

Thank you again! 🎉

sanchezweezer commented 5 years ago

That's great to hear! 👍 For new version I create new pull request later :)

mariusandra commented 5 years ago

Feel free to make a PR for whatever needs to be added. However I already merged the changes to 1.0 and added a test to make sure it works. :)

So you can basically go and use it already if you're using kea 1.0. 🎉

sanchezweezer commented 5 years ago

Wow! You are fast :)