-
Notifications
You must be signed in to change notification settings - Fork 267
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
Use Vite in templates and examples #1912
Conversation
We detected some changes in |
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
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.
{name: 'vite', token: '1000014892'}, | ||
{name: 'subscriptions', token: '1000014928'}, | ||
{name: 'classic-remix', token: '1000014892'}, | ||
{name: 'metaobjects', token: '1000014928'}, |
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.
Was subscriptions
incorrect? Intentionally change the name to metaobjects
?
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.
Yes, it's not an example that uses --diff
. And the build script in this workflow uses --diff
so we should only list here --diff
examples, I think?
We were lucky subscriptions
worked but it may break 😅
examples/classic-remix/README.md
Outdated
# Hydrogen template: Classic Remix | ||
|
||
Hydrogen is Shopify’s stack for headless commerce. Hydrogen is designed to dovetail with [Remix](https://remix.run/), Shopify’s full stack web framework. This template contains a **minimal setup** of components, queries and tooling to get started with Hydrogen and the classic Remix compiler (i.e. before Vite). | ||
|
||
[Check out Hydrogen docs](https://shopify.dev/custom-storefronts/hydrogen) | ||
[Get familiar with Remix](https://remix.run/docs/en/v1) |
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.
Should there be more description about what "classic remix" is?
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.
Added a link to Remix docs 👍
"typecheck": "tsc --noEmit", | ||
"codegen": "shopify hydrogen codegen" | ||
}, | ||
"h2:diff": { |
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'm not familiar with this new property, I assume this is configuration when the diff is applied, the the vite files in the skeleton template don't end up in the output of this example. Very cool.
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.
Yeah, just a quick escape hatch for a few examples due to the migration from ESBuild to Vite. We can probably remove it when we move completely to Vite and remove the classic compiler 🤔
}); | ||
}; | ||
|
||
if (await hasViteConfig(directory ?? process.cwd())) { |
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.
Ah so make h2 build
automatically detect if there's a vite config and pick the build path from there.
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.
Yes. This is a "soft-mergeof
devand
dev-vite`. I'll make a new PR reorganizing this code better.
`"name": "example-classic-remix"`, | ||
); | ||
|
||
// ---- DEV |
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.
Nit picky, but would it be better to separate these out, so it's more obvious when one fails? Not a big deal if you prefer to keep them all together.
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've been thinking about this and would like to split this file in different ones. But I think this could still be here.
The ones I want to extract are the "build" and "dev" tests.
These should be in build.test
and dev.test
even if they use runInit
to generate fixtures 🤔
For a different PR, tho.
expect(mb).toBeGreaterThan(0); | ||
expect(mb).toBeLessThan(1); |
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 is great! We'll find a failure if we go over 1mb in the skeleton template output.
This was fixed it in #1891 👍
Nice catch, fixed by replacing |
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.
@frandiox while the error has gone when running h2 setup markets
inside the skeleton template, when selecting subfolders
, it only updates server.ts
, the route folder structure itself doesn't change. Is that intentional? If so, then 🟢 from me.
Yes, I think we never implemented this because you also need to modify other things in the files to handle the new param. |
WHY are these changes introduced?
We want to dogfood Vite before the stable release.
WHAT is this pull request doing?
--diff
examplesexample/vite
projectexample/classic-remix
projectdev
andbuild
commands so that we can runh2 dev
instead ofh2 dev-vite
. This merge will need more work a different PR.preview
anddeploy
commands to make them work with both Vite and Classic. This will also require another PR for cleanup.h2 setup
andh2 setup markets
commands to work with Vite.HOW to test your changes?
Run
dev
,build
,preview
,deploy
commands in skeleton and examples. It should all work 🤞