search

redfin / react-server

:rocket: Blazing fast page load and seamless navigation.
https://react-server.io/
Apache License 2.0
3.89k stars 184 forks source link

Remove __reactServerBase, __setReactServerBase and __reactServerManifest from window #800

Open jhnns opened 7 years ago

jhnns commented 7 years ago

Following up #794

I don't think that it's necessary to put these things on the window object. Most of it could be solved via webpack or the chunk-manifest-webpack-plugin (which is currently listed in the package.json but not used). I don't understand why this was necessary in the first place, so maybe you could shed some light on it?

I think, removing these implicit global variables makes it easier to grasp what is actually going on in the browser. I would even argue that beside __reactServerState there should nothing be on the window object. But that's clearly beyond my scope.

doug-wade commented 7 years ago

If I understand https://github.com/redfin/react-server/pull/96#discussion-diff-60105576 correctly, this is a good idea that we haven't implemented yet.

jhnns commented 7 years ago

I think, we will need to put one manifest on the window object because this needs to be rendered into the HTML (which is not long-term cached). And if something is rendered into the HTML, it can only be accessed via window from the bundle.

But we should use the chunk-manifest-webpack-plugin for this task.

roblg commented 7 years ago

(edited; I read some code and my thoughts clarified ;) )

__reactServerBase, __setReactServerBase are being used to set the domain from which static assets (webpack JS and CSS) are loaded. Since we're generating the JS/CSS react-server middleware at runtime, I suppose it's possible to seed those classes with the appropriate value from server startup params. It seems like we might want to use them in other places though? e.g., if we wanted to reference some image that lives on the CDN next to the webpack assets, but wasn't rolled up by webpack CSS, we'd want that base URL. I suppose you could argue that react-server shouldn't care about that, and clients of react-server could add their own globals for those values if necessary.

In master, __reactServerManifest does hold the output of chunk-manifest-webpack-plugin -- it's built in compileClient.js, and then passed to coreJsMiddleware. It's possible we may have misunderstood how chunk-manifest-webpack-plugin works -- if we can get it included in the bundle somehow in a way that doesn't unnecessarily regenerate hashes when unrelated things change, I think it makes sense to use it as intended, rather than circumventing it. :)

doug-wade commented 7 years ago

Disclaimer: haven't thought about this one very carefully.

Maybe we can put them into config.js, rather than attaching them to window?