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 Support for Node and Add Docs for Node and SSR #15

Merged
merged 5 commits into from
Jul 27, 2019
Merged

Conversation

agilgur5
Copy link
Owner

@agilgur5 agilgur5 commented Jul 27, 2019

  • (fix): do not error if 'window' is undefined
  • (fix): throw an error if no storage is configured
  • (docs): re-word storage section for clarity
  • (docs): note redux-persist's Storage Engines compatibility
  • (docs): add section on Node and SSR Usage

Replaces #6 and #9 and resolves #13

The gotchas in #9 were a bit too confusing for me to really be okay with merging such functionality in. On top of that, once I discovered that redux-persist's Storage Engines were also compatible with mst-persist (per #13), it meant that mst-persist could already support Node environments very easily, so I wouldn't want to hamper that type of usage by making it extremely unintuitive.
I think asking developers to put a conditional check in their own source code to handle SSR without server-side hydration is much simpler and less confusing, and therefore likely worthwhile even if it entails a bit more thought & work out-of-the-box.

barbalex and others added 2 commits July 27, 2019 16:48
- in certain environments like Node, 'window' will not be defined and
  so will cause mst-persist to error out on its check for localStorge,
  even if a different storage engine were configured

- this fix enables usage in Node environments, supporting usage like
  hydrating server-side
- if the localStorage default is unsupported in the current
  environment (e.g. Node), then it should throw with a specific
  error instead of continuing and unexpectedly throwing on getItem
  - will still error out, so not a "fix" in that sense, but it now will
    throw a better, more understandable error
Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading the README rendered GitHub-style in the branch view, I think docs could be worded more clearly and formatted a bit more consistently.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could keep tweaking wording, but I think this looks good now and makes sense / is intuitive to read through.

README.md Outdated Show resolved Hide resolved
- redux-persist's Storage Engines all follow the same localForage-style
  async Promise API, so they should all be compatible with mst-persist
  as well
  - docs should mention this as the interoperability adds many more
    usage options for developers
    - e.g. using node-storage or cookie-storage for Node or universal
      apps, which should now work without bugs
- note that one can hydrate server-side with Node and give some example
  storage engines one may use for that purpose
- note that server-side hydartion can be skipped for certain use cases
  like SSR and give example for how to do so
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly or is fixed scope: docs Documentation could be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs should mention that mst-persist is compatible with redux-persist's Storage Engines
2 participants