-
Notifications
You must be signed in to change notification settings - Fork 286
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
Support Remix v2 in route generator #756
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also create a doc in the repo for us, to outline how we're supposed to format our exports to ensure that this works? For example:
- use `{name}v1` for v1
- use `{name}` for v2
or something like that?
@@ -31,7 +33,7 @@ describe('generate/route', () => { | |||
}); | |||
|
|||
// When | |||
await runGenerate(route, { | |||
await runGenerate(route, route, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This double route
thing confused me for a bit. The second one is routeTo
, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yesss, that's correct. I want to refactor this code in another PR to make it more testable, and will also change the signature.
|
||
function convertToMetaV1(template: string) { | ||
return template | ||
.replace(/type V2_MetaFunction\s*,?/, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this doesn't seem too bad, honestly.
However, I also found https://github.com/unjs/magicast the other day. Do you think it would be worth it to explore using that, or not really?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw magicast on Twitter yesterday but didn't want to add another dependency for this simple usecase. If we end up having more complex migration then we'll give it a try but, for now, I think REs are enough 🤔
@frehner I've added some docs to the skeleton readme 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, thank you
Related: #747
WHY are these changes introduced?
Remix has future flags that change the way routes work in a project:
v2_routeConvention
,v2_meta
andv2_errorBoundary
.So far, our generator logic only supports v1.
WHAT is this pull request doing?
With this PR, we check if any of the supported flags is enabled in the project and change the way we generate new routes.
If
v2_routeConvention
is enabled, the new files are generated in the corresponding path for v2. For example,routes/_index.tsx
instead ofroutes/index.tsx
.If
v2_meta
orv2_errorBoundary
are enabled, the corresponding exports in route files are adapted to v2 format (see the test file for more info).Since v2 future flags are now supported in full in Hydrogen, I've also enabled these flags in the
hello-world
template so that new projects start using Remix v2 features.HOW to test your changes?
From the hello-world template, run
npx shopify hydrogen generate routes
and check that they all work (v1). Then, remove all the routes, enable the mentioned flags inremix.config.js#future
, and run the command again. The files should have different filepaths, and some of them should contain v2 version of the meta export (policies and page).Expected review
Would like to know:
Checklist