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

Minimal SSR fix. #344

Conversation

microbit-matt-hillsdon
Copy link
Contributor

@microbit-matt-hillsdon microbit-matt-hillsdon commented Mar 16, 2020

See e.g. webpack/webpack#6522 /
webpack/webpack#5843
and the docs at
https://webpack.js.org/configuration/output/#outputglobalobject

This was broken by the Webpack 4 upgrade in #273.
This fix effectively reinstates Webpack 3 behaviour, using this rather than assuming window.
In node it'll be using module.exports anyway, we just need not to explode because of window + strict mode.

Note the component still doesn't really support SSR, but
its calls to Browser APIs are in componentDidMount and similar
so are delayed until client side.

Todos pending approval of approach

  • package.json:version has been updated with npm version patch (or major/minor as appropriate)
  • @brainhubeu/react-carousel version has been updated in docs-www/package.json to the same which is in package.json:version

See e.g. webpack/webpack#6522 /
webpack/webpack#5843
and the docs at
https://webpack.js.org/configuration/output/#outputglobalobject

This was broken by the Webpack 4 upgrade in brainhubeu#273.
This fix effectively reinstates Webpack 3 behaviour in the browser.

Note the component still doesn't really support SSR, but
its calls to Browser APIs are in componentDidMount and similar
so are delayed until client side.
This was referenced Mar 16, 2020
@piotr-s-brainhub
Copy link
Contributor

@microbit-matt-hillsdon

thanks for this PR but I don't understand what is the benefit as it doesn't provide SSR

@microbit-matt-hillsdon
Copy link
Contributor Author

@microbit-matt-hillsdon

thanks for this PR but I don't understand what is the benefit as it doesn't provide SSR

@piotr-s-brainhub, thanks for taking a look.

In 1.12.0 I was able to use this component just like any other in my Gatsby project. It didn't render anything until after componentDidMount, but that was acceptable to me for a dynamic component like this. I could wrap it to display a placeholder or just accept that it would render nothing server side. I'd say at this point it was SSR-friendly, even if it didn't render until client side.

In 1.12.1 (after #273), this was no longer possible as the module wrapper accessed window at import time (due to webpack v4 UMD module default behaviour changes) so my Gatsby build failed. From the variety of workarounds suggested on #287, I guess I'm not the only one that cares about this configuration.

If I find time in future, I would also be interested looking at how hard it would be to render something server side too, but for now this simple fix will get users back to where they were with 1.12.0.

I'd love to see this merged to make the code SSR-friendly again.

@piotr-s-brainhub piotr-s-brainhub changed the base branch from master to microbit-matt-hillsdon-minimal-ssr-fix March 18, 2020 12:57
@piotr-s-brainhub piotr-s-brainhub merged commit 710eab6 into brainhubeu:microbit-matt-hillsdon-minimal-ssr-fix Mar 18, 2020
@piotr-s-brainhub
Copy link
Contributor

@microbit-matt-hillsdon

I opened #352 instead as only the main repo allows building testing environment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants