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

(feat): support SSR out-of-the-box by no-op'ing in Node #9

Closed
wants to merge 3 commits into from

Conversation

agilgur5
Copy link
Owner

  • add a nodeNoop option that is true by default
    • generally folks do not want to hydrate their store during SSR, so
      no-op by default and require explicit opt-in to Node hydration
    • see enable server side rendering #6 for discussion
  • also throw an Error if localStorage isn't supported and no
    alternative storage provider has been given

(docs): add nodeNoop option and section on Node and SSR usage to
give more details and note possible gotchas


Replaces #6

After actually writing the docs for the gotchas, I'm on the fence about implementing this. The ramifications for test and other environments are not ideal either... Need to give it some more thought / consideration or discussion from more folks 💭

@agilgur5
Copy link
Owner Author

Not sure why I didn't think of this, but I could make nodeNoop check if the environment is test to remedy the testing differences.

Also the error out on unsupported part should be split out into a separate commit. And it should ideally go into a patch release on its own.

agilgur5 added 3 commits July 13, 2019 21:30
- 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" per se, but it now will
    throw a better, more understandable error
- add a `nodeNoop` option that is `true` by default
  - generally folks do not want to hydrate their store during SSR, so
    no-op by default and require explicit opt-in to Node hydration
  - see #6 for discussion

(docs): add `nodeNoop` option and section on Node and SSR usage to
give more details and note possible gotchas
- without this, the option wouldn't be fully backward compatible and
  would break existing tests run in Node (without 'window' global)
  - i.e. those that use a universal storage engine
- no need to configure `nodeNoop` to `false` just for tests

FIXME: UPSTREAM BLOCKER: the CJS development build replaces
process.env.NODE_ENV with 'development' and there is no 'test' build...
- so this only works with ESM builds which generally aren't supported
  in test envs either.... :/ :/ :/ :/
@agilgur5 agilgur5 force-pushed the node-and-ssr-support branch from 83320e9 to 65eb7a8 Compare July 14, 2019 02:02
@agilgur5
Copy link
Owner Author

Oof the more I work on this PR the more it seems like a bad idea. The code and options are just confusing, tbh. I'm not sure if either the commits for nodeNoop: true makes sense or if nodeNoop: false if process.env.NODE_ENV === 'test' makes sense. 😕

Also, the CJS builds output by tsdx don't have a test build, and the development build is transforms process.env.NODE_ENV to 'development', so the env check becomes always false in CJS tests. And since most tests are run in Node without ESM support (see #3), this means that check has no effect. The ESM build includes process.env.NODE_ENV as is, with no replacement, oddly enough.

@agilgur5
Copy link
Owner Author

Closing this out in favor of #15 per the rationale listed there (less confusing, less gotchas, more intuitive, Node can already be supported).

Was thinking of possibly adding a note in the "Node and SSR Usage" docs about this PR as a rejected proposal in order to be clear an decision was made due to the trade-offs listed here. It would be for those wondering why it's not supported "out-of-the-box" as well as to promote discussion here. But given that mst-persist supports server-side/Node hydration, I think it's a bit implicit that "out-of-the-box" support for a non-hydration default isn't possible or inherently comes with trade-offs. And this has enough gotchas on top of that that I'm not sure promoting any discussion would really be useful -- #15 is likely the optimal option.

@agilgur5 agilgur5 closed this Jul 27, 2019
@agilgur5 agilgur5 deleted the node-and-ssr-support branch July 27, 2019 21:29
@agilgur5 agilgur5 added the kind: feature New feature or request label Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request progress: external blocker This is blocked by an issue in an external dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant