Closed trevordmiller closed 7 years ago
@joelhooks @ijones16 @tayiorbeii Does this look ok? As far as I know, the only other issues that would prevent egghead-rails from upgrading to latest egghead-ui are the small breaking changes in <Button>
(the size
values have changed, and pill
is no longer a prop: https://styleguide.egghead.io/components/Button)
Hold off reviewing this, I'm going to add a test that renders a server importing the egghead-ui library to ensure these changes actually fix the problems and prevent adding more in the future
@joelhooks @ijones16 @tayiorbeii Ok this is ready for review now. I've updated the description with more info. I started going down a rabbit hole (set up a next server and webdriverIO etc. and then realized all that was needed to test server rendering is no errors on ReactDOMServer.renderToString
🤦♂️ ). This PR should fix the initial server-rendered runtime errors and prevent new ones in the future.
For future reference:
So the gist of it is, a bunch of stuff is available globally (like setTimeout
, setInterval
etc.) but the global space is different depending on if the code is being run on the server (node) or browser; things like localStorage
are actually a shorthand for window.localStorage
, where window
only exists when running in the browser. Because egghead-rails runs on the server for the first render, and then on the browser for the next renders, window
isn't defined so it creates a runtime error. This is why I've added the conditionals like const window = window || false
and if (!localStorage) return
etc.
The node global is called global
which has some of the same things as window
, but only things that are not specific to the DOM. So you can do setInterval(...)
in both node and browser (since global.setInterval
and window.setInterval
both exist), but you can't do dom specific things like localStorage(...)
because global.localStorage
doesn't exist, it's only on window.localStorage
.
Moral of the story is: globals are messy and it's better to use npm modules that work on the server and browser wherever possible, but in our case, I had to add some gross conditionals so the initial render can occur on the server without any runtime errors
Changes
egghead-ui package
), so pull requests will fail if any client-side only logic is addedResult For User
New test fails if there are any rendering errors when rendering all resources on the server
After fixing client-side only logic, these pass
Note that logs still show which is why you see the warning from prop-types (I'll be fixing this in another story). The tests only fail when there is an exit code, which is what happens if any of the resources can't render on the server.