✨💥 pass data, url to app component by default in react-server-webpack-plugin#1423
✨💥 pass data, url to app component by default in react-server-webpack-plugin#1423
Conversation
| import App from 'index'; | ||
| import {showPage, getSerialized} from '@shopify/react-html'; | ||
|
|
||
| import App from './index'; |
There was a problem hiding this comment.
this line is failing for me as it cannot find ./index
There was a problem hiding this comment.
🤔 I thought I had left this as 'index' instead of './index', if you change it back does it work?
There was a problem hiding this comment.
yup changing it back to index works fine.
There was a problem hiding this comment.
Yah, I recall that the webpack plugin needed index instead of ./index. It was a weird thing I explored/debugged ~8-10 months ago when we first started the plugin.
packages/react-server-webpack-plugin/src/react-server-webpack-plugin.ts
Outdated
Show resolved
Hide resolved
| return <div>this is an about page</div>; | ||
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
running into a small integration issue with react-router
import {Router} from '@shopify/react-router';
<Router location={url}>
👆 this fail type check because location is typed location?: string;
we can use url.href or change the type to location?: string | URL; and everything would still work
There was a problem hiding this comment.
url.href is the right choice, that's what I did in the branch I linked
There was a problem hiding this comment.
Yes, I understand that I can use url.href. The point I was trying to make it we don't have to.
Our Router only use location to pass to react-router's StaticRouter
Their doc show example of the following
<StaticRouter location={req.url} context={context}>
<App />
</StaticRouter>
which mean with a type change Router can accept object as well as string.
There was a problem hiding this comment.
ahhh, sorry I misunderstood your comment. Seems like it makes sense to update the type indeed then!
|
A thought I am having as I integrate it with the magic provider. Once we have |
| <NetworkContext.Provider value={networkManager}> | ||
| {children} | ||
| <Serialize data={() => ctx.headers['x-quilt-data']} /> | ||
| <Serialize data={() => ctx.state.quiltData} /> |
There was a problem hiding this comment.
I don't see a changelog update for this so I presume it doesn't need one. No user impact so makes sense.
|
|
||
| ### Changed | ||
|
|
||
| - Generated entrypoints no longer render the App component with a `location` prop. Apps can instead get the same data from the new `url` prop's `href` attribute. |
There was a problem hiding this comment.
This would constitute a breaking change. Can we highlight this more with some
There was a problem hiding this comment.
These changes make sense to me, 🎩 checks out as well.
One thing that concerns me is that the breaking changes are hidden in the changelog for react-server-webpack-plugin which wouldn't be the first place I'd look if I noticed a breaking change.
Maybe we can draw inspiration from sewing-kit's migration guide doc? But it'll be a bit trickier with quilt_rails related libs since they're all on different versions. This is probably worth a later discussion though.
|
@ismail-syed probably the relevant spot for more migration explanation would be sewing-kit since it will be hooking up the plugin? |
Good point. Publishing this as a breaking change would address all the concerns I had in mind. |
marutypes
force-pushed
the
pass-url-and-data-through-webpack
branch
from
be3c491 to
36a969a
Compare
michenly
left a comment
michenly
left a comment
There was a problem hiding this comment.
Me & @TheMallen chatted about including react-router in this PR. I am good with it not being included now.
…plugin.ts Co-authored-by: Michelle Chen <michelle.chen@shopify.com>
marutypes
force-pushed
the
pass-url-and-data-through-webpack
branch
from
36a969a to
c930991
Compare
3 participants
Closes #1115
What do
This PR revamps what we pass into the user's app in our react-server plugin. It is a breaking change, and since we
In detail it:
x-quilt-dataheader parsing into it's own middlewarex-quilt-dataserialization key to justquilt-data(no need for x when it is no longer a header)URLobject into the app component in@shopify/react-server-webpack-pluginas an automatic propquilt-datainto the app component as wellTophatting 🎩
It's tough to write sane tests for this, but I tophatted this pretty thoroughly using
hydrox-againfollowing this method:yarn buildcp ./packages/react-server ../hydrox-again/node_modules/@shopifycp ./packages/react-server-webpack-plugin ../hydrox-again/node_modules/@shopifycp ./packages/react-server-webpack-plugin ../hydrox-again/node_modules/@shopify/sewing-kit/node_modulesApp.tsxto use the new propsI also prepared this branch that makes all the changes necessary to get this working and also removes some cruft. Once this is actually released we can use this branch to actually install the new version.