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

agilgur5 / mst-persist

Persist and hydrate MobX-state-tree stores (in < 100 LoC)

Other

87 stars 17 forks source link

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

Closed agilgur5 closed 5 years ago

agilgur5 commented 5 years ago

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
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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.