Change style-loader to something SSR-ready #3087
Conversation
LogERROR ON TASK: unitTests ERROR ON TASK: scaffoldingSucceeds |
jimbo
added
the
version: Major
There was a problem hiding this comment.
Spotted some hard-coded values that need resolved, and a few TODO's, but overall the re-factor to useStyle looks good. We've picked up additional mergeClasses calls from mainline that'll need updated, but probably best to wait until this is ready for delivery, and update once more.
packages/venia-concept/src/index.js
Outdated
| const { BrowserPersistence } = Util; | ||
| const apiBase = new URL('/graphql', location.origin).toString(); | ||
| const origin = 'https://magento-venia-concept-e1r4b.local.pwadev:8048'; |
There was a problem hiding this comment.
This hard-coded value likely only works on your local, this will need some SSR treatment as well since location.origin will not be available. Pretty sure you won't have access to useLocation outside of the router context either, so some sort of communication from UPWARD would need to inform Apollo of its origin. React Router docs also seem to indicate the need to use the stateless StaticRouter instead of BrowserRouter, should that be in this scope?
There was a problem hiding this comment.
True, I should have reworked this. The server will have to provide the origin when it executes the script, so we'll probably want to pull parts of src/index.js into a separate function that takes origin as an argument. I'll make that change tomorrow.
As for StaticRouter, yes, it'll have to be done later. There's a fair bit more work to be done to get the server bundle to actually render something, but this work is just to let it run without throwing.
There was a problem hiding this comment.
@tjwiebell Okay. I've refactored Adapter to follow our talon-component pattern, slimmed down the index.js entrypoint, and set origin to MAGENTO_BACKEND_URL on the server for now. When we dig into the real SSR work and have an express handler, we can find a way to retrieve origin from the HTTP request and get it to our script.
| }); | ||
|
|
||
| // remove HtmlWebpackPlugin | ||
| serverConfig.plugins.pop(); |
There was a problem hiding this comment.
Is this position guaranteed? Seems a little fragile, but comfortable with this if we control both sides.
There was a problem hiding this comment.
No; it's not guaranteed, and it is fragile. I've updated the code to filter by plugin name rather than index.
m2-community-project
bot
added
Progress: changes requested
and removed
Progress: ready for review
labels
m2-community-project
bot
added
Progress: review
and removed
Progress: changes requested
labels
There was a problem hiding this comment.
This looks good to merge into the source branch, love the adapter abstraction and how clean the component looks with prop collections. There are some failures related to scaffolding, but I'm comfortable merging this and we'll get everything passing in #2991.
Description
In the course of removing (or guarding)
windowand other browser-only global references from the codebase, it became clear that a few references existed in dependency code. One such dependency isstyle-loader, a standard Webpack loader that inserts CSS into<style>elements. The maintainers ofstyle-loaderappear to be aware of the issue, but have closed it aswon't-fix, so we need to replace it.The closest server-compatible loader I can find is
isomorphic-style-loader; while it has some minor flaws (hard dependency onreact) and doesn't appear to be actively maintained anymore, it is MIT licensed, so we should be able to incorporate it with proper attribution. I've tried to do so in this PR.The largest change proposed here is a switch from
mergeClassesto a new hook (useStyle) with context, which allows style accumulation and insertion to work differently between browser and server.Related Issue
Merges into #2991
Acceptance
Verification Stakeholders
Specification
Verification Steps
yarn watch:venia).develop.Screenshots / Screen Captures (if appropriate)
Checklist