enable server side rendering #6

barbalex · 2019-07-07T21:02:41Z

I use GatsbyJS in my App. Which means that as soon as mst-persist is imported, the gatsby project can not be built any more: the build errors window is not defined.

So I now have to use mst-persist like this:

  if (typeof window !== 'undefined') {
    import('mst-persist').then(module =>
      module
        .default('store', store, {
          storage: localForage,
          jsonify: false,
        })
        .then(() => {
          console.log('store has been hydrated')
          // navigate to last activeNodeArray
          store.tree.setActiveNodeArray(store.tree.activeNodeArray)
        }),
    )
  }

Thus it is much easier if if (typeof window !== 'undefined') is done in mst-persist itself.

agilgur5 · 2019-07-08T05:35:15Z

Thanks for the contribution @barbalex 😃

The example code you've given seems a bit unnecessary (or not minimal?) -- you don't need to dynamically import to get around the non-existent window, you can import persist as normal and only call it within the if statement. See also pinqy520/mobx-persist#29 that presents a similar error and a workaround for it.

i.e. you could do:

import { persist } from 'mst-persist'

...

if (typeof window !== 'undefined') {
  persist('store', store, {
    storage: localForage,
    jsonify: false,
  }).then(() => {
    console.log('store has been hydrated')
    ...
  })
}

The code you added in this PR potentially makes sense for non-browser support, but not really for SSR -- you're not going to want to hydrate the store on the server anyway. That would be accessing your server's storage instead of the browser's.

I'd be happy to add this for NodeJS support, but you'll need an if statement to check that you're in the browser environment in any case, so that wouldn't actually solve your specific issue.
In fact, if I do add this for NodeJS support, I feel like many other people might accidentally not realize they're accessing server-side storage during SSR too and so it should be called out explicitly in the docs (or maybe a warning if it can detect SSR vs just server usage??). Actually, it sounds like a big enough gotcha that it might just make sense to not support NodeJS at all as MobX probably has 99%+ usage browser-side.
redux-persist does support Node with its cookie-storage adaptor (so perhaps something to support in the future), though it still has issues with unconditional SSR, so even in redux-persist one would need to check which environment one is in in user code.

agilgur5 · 2019-07-08T07:36:24Z

Re-reading and I see once more that you wrote the build fails ("on import"). The check for window.localStorage only occurs at run-time however, so I'm a bit confused by that. The mobx-persist user who opened their SSR issue linked above also did not seem to have any build issues -- this might be a tooling-specific error.

It sounds like Gatsby might be trying to make a Node-only build and during transpilation or bundling has some issues? I'm still not sure why it would be running that code however, since it's not top-level. What's the error you're getting?

barbalex · 2019-07-08T22:56:21Z

you can import persist as normal and only call it within the if statement

Yes, that should work. And often it does. But sometimes it has blown up in my face in the past and also when first implementing mst-persist. I do not know why this happens but I am not taking any more risks as failing builds can be really bad if it happens after changing db structure and you need to build on time. I suspect there are edge cases when the window reference in a library provokes the error purely by importing the library. But I can not explain this behavior. Anyway: If the library has no unchecked window reference this will not happen and then I can import it directly.

makes sense for non-browser support, but not really for SSR

I did not mean SSR. Rather: Gatsby builds the project in node (gatsbyjs/gatsby#13355 (comment)). And that is why window references do not work.

I'd be happy to add this for NodeJS support, but you'll need an if statement to check that you're in the browser environment in any case, so that wouldn't actually solve your specific issue

I don't understand why I would have to check that I am in the browser environment: Gatsby apps always run in the browser. But they build in node.

Hm. You mean: I would not want to rehydrate during SSR so I would have to leave my code as it is (but I could import mst-persist directly) in order to not rehydrate server side. O.k., that probably makes sense - I am a complete noob when it comes to SSR.

But it would still be nice if a gatsby build would not break and force users to debug that.

What's the error you're getting?

$ gatsby clean && gatsby build
info Deleting .cache, public
info Successfully deleted directories

success open and validate gatsby-configs - 0.018 s
success load plugins - 1.591 s
success onPreInit - 0.004 s
success delete html and css files from previous builds - 0.005 s
success initialize cache - 0.007 s
success copy gatsby files - 0.107 s
success onPreBootstrap - 0.013 s
success source and transform nodes - 0.296 s
success building schema - 0.265 s
success createPages - 0.053 s
success createPagesStatefully - 0.044 s
success onPreExtractQueries - 0.004 s
success update schema - 0.018 s
success extract queries from components - 0.449 s
success write out requires - 0.006 s
success write out redirect data - 0.002 s
success Build manifest and related icons - 1.229 s
success onPostBootstrap - 1.239 s
⠀
info bootstrap finished - 8.035 s
⠀
success run static queries - 0.010 s — 1/1 145.14 queries/second
success Building production JavaScript and CSS bundles - 29.421 s
success Rewriting compilation hashes - 0.002 s
success run page queries - 2.598 s — 37/37 14.28 queries/second
success Generating image thumbnails — 43/43 - 5.157 s
⠇ Building static HTML for pages
⠦ Building static HTML for pages
(node:20912) UnhandledPromiseRejectionWarning: ReferenceError: window is not defined
    at Module.persist [as default] (C:\Users\alexa\apf2\public\render-page.js:149750:3)
    at C:\Users\alexa\apf2\public\render-page.js:213343:26
    at processTicksAndRejections (internal/process/task_queues.js:86:5)

The node:20912-Error gets repeated about a hundred times . It does not point to any specific location so if you are not familiar to what is happening it is a bit oblique.

agilgur5 · 2019-07-09T01:21:19Z

Hm. You mean: I would not want to rehydrate during SSR so I would have to leave my code as it is (but I could import mst-persist directly) in order to not rehydrate server side. O.k., that probably makes sense - I am a complete noob when it comes to SSR.

Yup, exactly! You still need to check if you're in browser or not so as to not rehydrate server-side.
It sounds like you're still getting occasional build issues even when the persist call is behind a window check though (as in my example), right?

The node:20912-Error gets repeated about a hundred times . It does not point to any specific location so if you are not familiar to what is happening it is a bit oblique.

Thanks for the error log. It fails on build because Gatsby isn't a server, it's a static site generator, but may use SSR mechanics in order to generate the static HTML (whereas a server would only error at runtime, Gatsby doesn't have a "runtime", the build is its runtime). Semantics, I suppose, but "at build-time" has a bit of a different meaning when you're not just transpiling / bundling, but creating HTML (a bit of an impedance mismatch). When I first saw Gatsby, I didn't immediately realize that and didn't realize it until I saw the log saying "Building static HTML" 😅

I think I understand what's going on. Because it's generating HTML, it is actually rendering all your components and running your client-side code. I'm guessing it's giving 1 error per page (or however many times a page is rendered)? That being said, importing directly and putting the persist call behind the window check as in my example should fix the error (shouldn't be a need to dynamic import)

Yes, that should work. And often it does. But sometimes it has blown up in my face in the past and also when first implementing mst-persist. [...] I suspect there are edge cases when the window reference in a library provokes the error purely by importing the library

And this is where I'm confused too. It definitely should work and definitely shouldn't be non-deterministic like that. The error is occurring at render-time (let's the use the term "render-time" instead of "build-time" to be more explicit), so it shouldn't pass the window check, and if it did, it shouldn't say window doesn't exist immediately after confirming it indeed exists.

The issue you referenced said it might be different in dev vs. prod builds. That might help narrow it down from something so non-deterministic as you observe.
Your error logs also say UnhandledPromiseRejectionWarning which makes it sound like the persist call is running, even if behind a window existence check (unless that's from a different Promise).
The workarounds in redux-persist are similar to the one in my example, although redux-persist/lib/storage has a side-effect on import, whereas mst-persist should have no side-effects on import and so only persist should need to be behind a window check, not the import itself.

Anyway: If the library has no unchecked window reference this will not happen and then I can import it directly.

Initially I was thinking of just merging it as there wouldn't seem to be any harm done (though you should be able to import directly), but the behavior is not necessarily ideal, because it means mst-persist will be able to run server-side (so long as you've given it a server usable storage) and in the majority of cases that would not be intentional.

Perhaps persist should no-op if window doesn't exist and then require another server: true option to explicitly opt-in to server-side usage. Though one would probably give it a different storage option on server-side anyway, so maybe something like that is redundant? I guess it would have "SSR no-op" support out-of-the-box that way though, just wouldn't do anything during SSR. So maybe that's the optimal solution here.

barbalex · 2019-07-09T09:09:32Z

Perhaps persist should no-op if window doesn't exist and then require another server: true option to explicitly opt-in to server-side usage

Sounds good. But to be honest: You obviously understand these processed a lot better so I am not going to try to influence you.

- 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

agilgur5 · 2019-07-11T03:56:59Z

Were you able to confirm that statically importing still gives you a render-time error even when persist is hidden behind a window check like the example I gave? Because if that's the case, these fixes shouldn't work either.

I've added a replacement PR in #9 that adds a nodeNoop option. It's also got docs and some error-checking as well and is up-to-date on top of the TS refactor that I just landed (which still needs to be published at time of writing).

The problem was that when I was writing the docs, I realized there are going to be some potential gotchas on testing and other environments due to the default no-op in Node, so I'm having second thoughts 😖. Need to look at what some other libraries that support SSR out-of-the-box and see how they handle the situation.

barbalex · 2019-07-11T08:07:13Z

Were you able to confirm that statically importing still gives you a render-time error even when persist is hidden behind a window check like the example I gave?

No. I changed the code to import it directly and the project built just fine. But that was only a single test. It had broken that way while implementing first. But I can't be 100% sure that was not caused by something different.

I had similar problems using react-leaflet. The problem there is that react-leaflet uses leaflet which actually manipulates the window object. I spent a rather long time trying to get my gatsby project to build again and found that the only working version was hiding the map components (that use react-leaflet) behind this component here:

import React, { useState } from 'react'

/**
 * do not load Karte on server
 * because leaflet calls windows
 * see: https://github.com/PaulLeCam/react-leaflet/issues/45#issuecomment-257712370
 */
export default ({ treeName }) => {
  const [Karte, setKarte] = useState(null)

  if (typeof window === 'undefined') return null

  import('./Karte').then(module => setKarte(module.default))

  if (!Karte) return null
  return <Karte treeName={treeName} />
}

As found in PaulLeCam/react-leaflet#45 the way to solve is to conditionally import. But this case may be different as leaflet does not only reference but manipulates the window object. Not sure if that makes a difference.

- 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

agilgur5 · 2019-07-14T02:53:44Z

No. I changed the code to import it directly and the project built just fine. But that was only a single test. It had broken that way while implementing first. But I can't be 100% sure that was not caused by something different.

Ok. That's a pretty important difference if you can't reproduce it. It is almost certainly something else and not this package.

As found in PaulLeCam/react-leaflet#45 the way to solve is to conditionally import. But this case may be different as leaflet does not only reference but manipulates the window object. Not sure if that makes a difference.

It doesn't necessarily matter whether it manipulates or not. Per that issue and the quoted README section: "Leaflet makes direct calls to the DOM when it is loaded" -- i.e. leaflet has side-effects that involve client-side only things. mst-persist does not have side-effects, so just importing it should not cause any errors, only actually calling persist server-side will.

Going to close this out. Implementation-specific discussion of mst-persist can continue in #9. The nodeNoop option there still seems extremely unintuitive (even with testing support by checking process.env.NODE_ENV === 'test'), so I'm still leaning on saying developers who need SSR should manually put a if (typeof window !== 'undefined') check in their code as that's more intuitive and explicit. Still need to think about more and get some more eyes on it (the testing support is also currently blocked by an issue in tsdx)

barbalex · 2019-07-14T12:03:08Z

@agilgur5 thanks for taking this so serious and for providing this great tool

agilgur5 · 2019-07-27T21:37:11Z

Made one more replacement PR for this in #15 that I'll be merging in shortly. It opts to just add docs for adding a conditional environment check, which is a lot more intuitive and doesn't have quite as many gotchas as #9, albeit requiring a bit more work on top of the "out-of-the-box" experience for SSR.

Due to my discovery noted in #13, mst-persist can support Node already with a different storage engine like one of redux-persist's as they're compatible with each other, so wouldn't want to hamper that functionality with the gotchas of #9 either.

@barbalex I also made sure you're credited for the code you added for Node support there.

enable server side rendering

803565f

agilgur5 mentioned this pull request Jul 11, 2019

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

Closed

agilgur5 closed this Jul 14, 2019

agilgur5 mentioned this pull request Jul 27, 2019

Fix Support for Node and Add Docs for Node and SSR #15

Merged

agilgur5 added the kind: bug Something isn't working properly or is fixed label Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable server side rendering #6

enable server side rendering #6

barbalex commented Jul 7, 2019

agilgur5 commented Jul 8, 2019 •

edited

Loading

agilgur5 commented Jul 8, 2019 •

edited

Loading

barbalex commented Jul 8, 2019 •

edited

Loading

agilgur5 commented Jul 9, 2019 •

edited

Loading

barbalex commented Jul 9, 2019

agilgur5 commented Jul 11, 2019

barbalex commented Jul 11, 2019

agilgur5 commented Jul 14, 2019 •

edited

Loading

barbalex commented Jul 14, 2019

agilgur5 commented Jul 27, 2019

enable server side rendering #6

enable server side rendering #6

Conversation

barbalex commented Jul 7, 2019

agilgur5 commented Jul 8, 2019 • edited Loading

agilgur5 commented Jul 8, 2019 • edited Loading

barbalex commented Jul 8, 2019 • edited Loading

agilgur5 commented Jul 9, 2019 • edited Loading

barbalex commented Jul 9, 2019

agilgur5 commented Jul 11, 2019

barbalex commented Jul 11, 2019

agilgur5 commented Jul 14, 2019 • edited Loading

barbalex commented Jul 14, 2019

agilgur5 commented Jul 27, 2019

agilgur5 commented Jul 8, 2019 •

edited

Loading

agilgur5 commented Jul 8, 2019 •

edited

Loading

barbalex commented Jul 8, 2019 •

edited

Loading

agilgur5 commented Jul 9, 2019 •

edited

Loading

agilgur5 commented Jul 14, 2019 •

edited

Loading