-
Notifications
You must be signed in to change notification settings - Fork 280
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
Integrate hydrogen-react into the hydrogen monorepo #605
Conversation
Add github workflows
* test to see if alex is breaking ci stuff * add alex back, and check changeset checker * downgrade alex, and add better words/docs on changeset reminder * revert alex downgrade * see if we can figure out where this is coming from * remove testing as it didn't find anything * remove diff command on alex * remove comment that tests changeset checker
* Fix readmes * update terminology
Add the variant source header for SFAPI calls.
* provide guidance on setting the header content type * language lint * change pkg name
* update readme * fix links Co-authored-by: Elise Yang <[email protected]>
* CI action for publishing the next version automatically * remove output to file
* CI action for publishing the next version automatically * remove output to file * Don't use the action to run these commands
* Change TS's module resolution to 'node' instead of nodenext * Include stories in TS checking now, and fix issues
* Copy CartProvider from hydrogen repo * Add worktop lib to use the cookies package for CartProvider * Modify CartProvider to fit hydrogen-ui * Add CartProvider story * add changeset * Cart provider updates (#14) * Change ts moduleresolution (#13) * Change TS's module resolution to 'node' instead of nodenext * Include stories in TS checking now, and fix issues * Starting work on simplifying the cart code in hui * Simply more cart-related files and remove a bunch of them * All CI checks are passing, and types are improved. * Better file names * Fix mock import filename * Remove any type * Improve release notes, update type names, fix typo * Correct alex config file and allow hooks Co-authored-by: Anthony Frehner <[email protected]>
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.
You can pretty much ignore all the files/change inside of the packages/hydrogen-react
folder, so that should help this PR be much smaller.
@@ -48,7 +48,7 @@ jobs: | |||
- name: 📥 Install dependencies | |||
run: npm ci | |||
|
|||
- name: 🔬 Lint | |||
- name: 🔬 Check Formatting |
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 in the prettier
job, so the name needed to be updated.
@@ -4,10 +4,11 @@ | |||
"scripts": { | |||
"build": "turbo build --parallel", | |||
"build:pkg": "npm run build -- --filter=./packages/*", | |||
"ci:checks": "turbo run lint test format:check typecheck", |
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.
new command that can be run from root, to run all ci checks locally
"dev": "npm run dev:pkg -- --filter=./templates/demo-store", | ||
"dev:pkg": "cross-env LOCAL_DEV=true turbo dev --parallel --filter=./packages/*", | ||
"preview": "turbo build --filter=./packages/* && npm run preview -w demo-store", | ||
"lint": "eslint --no-error-on-unmatched-pattern --ext .js,.ts,.jsx,.tsx ./packages && npm run lint -w demo-store", | ||
"lint": "eslint --no-error-on-unmatched-pattern --ext .js,.ts,.jsx,.tsx ./packages & npm run lint -w demo-store", |
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.
run the scripts in parallel instead of serially
@@ -4,19 +4,18 @@ import {fileURLToPath} from 'url'; | |||
|
|||
(async () => { | |||
const root = fileURLToPath(new URL('..', import.meta.url)); | |||
const hydrogenReact = 'node_modules/@shopify/hydrogen-react'; | |||
const hydrogenReact = 'packages/hydrogen-react'; |
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.
small updates to this script to get it working with the local files
@@ -30,6 +30,9 @@ | |||
"@shopify/create-hydrogen#build": { | |||
"dependsOn": ["@shopify/cli-hydrogen#build"] | |||
}, | |||
"@shopify/hydrogen#build": { | |||
"dependsOn": ["@shopify/hydrogen-react#build"] | |||
}, |
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.
hydrogen requires hydrogen-react to build first
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.
For the types?
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.
For everything; hydrogen re-exports things from hydrogen-react, so that will only work if hydrogen-react has been built first.
npm exec --workspaces -- npx rimraf node_modules && npx rimraf node_modules |
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.
Local testing went smoothly 🎉
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.
Great work @frehner! I tophatted with ladle and everything seems to work alright.
README.md
Outdated
|
||
**Requirements:** | ||
|
||
- Node.js version 16.14.0 or higher | ||
- `npm`, `yarn` or `pnpm` | ||
- `npm` version 8.3.1 (or your package manager of choice, such as `yarn` or `pnpm`) |
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.
Why do we need this specific version of npm? 🤔
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 was an addition by @gfscott ; thoughts?
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 would not add npm version, unless there was a requirement for a certain version
This reverts commit 7ef939e.
@frehner wondering if it still makes sense to be using vite? Just a question for future discussion, not a blocker for this PR. |
Yeah it's a good and valid question. I used Vite originally because that's what v1 was using, but now that v2 uses tsup it's worth looking into whether tsup supports all the things we need to do to bundle hydrogen-react, and if not, is Vite still the best tool or should we use something like Rollup or microbundle or whatever. |
Co-authored-by: Daniel Rios <[email protected]>
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.
Thanks, LGTM!
@@ -14,7 +14,7 @@ | |||
"scripts": { | |||
"build": "tsup --clean --config ../../tsup.config.ts && npm run copy-hydrogen-react", | |||
"copy-hydrogen-react": "node ../../scripts/copy-hydrogen-react.mjs", | |||
"dev": "tsup --watch --config ../../tsup.config.ts", | |||
"dev": "tsup --watch --config ../../tsup.config.ts --watch ../../node_modules/@shopify/hydrogen-react/dist/browser-prod/index.mjs", |
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.
maybe ../hydrogen-react/dist/browser-prod/index.mjs
better? The node_modules one should be a symlink to this one.
Also, what's this for? Types? 🤔
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 helped hydrogen reload when hydrogen-react is updated. It could be that it's not needed, I'll verify, but we had a hard time getting it to work correctly and this is what we ended up with.
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.
Confirmed that this is necessary to get reloading working (at least with the way it is setup now).
I'll merge with this, but improvements as followups are very welcome!
@@ -30,6 +30,9 @@ | |||
"@shopify/create-hydrogen#build": { | |||
"dependsOn": ["@shopify/cli-hydrogen#build"] | |||
}, | |||
"@shopify/hydrogen#build": { | |||
"dependsOn": ["@shopify/hydrogen-react#build"] | |||
}, |
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.
For the types?
This reverts commit d85ba01.
WHY are these changes introduced?
This is the majority of the work to integrate hydrogen-react back into the hydrogen repo. Namely:
ci:checks
) that enables you to quickly run all the ci checks locally; before, you had to know which scripts were run in CI, and then run then individually.A big thank you to @wizardlyhel @juanpprieto and @blittle for pairing on this throughout various points as well!
Final questions:
HOW to test your changes?
npm i
,npm run dev
,Also:
npm run dev:story
) in the hydrogen-react package and ensure that it is working as well. (This helps you develop components in isolation, outside of remix)Post-merge steps
Checklist