-
Notifications
You must be signed in to change notification settings - Fork 144
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
[Theme] Change how assets are resolved #5252
Conversation
/snapit |
🫰✨ Thanks @frandiox! Your snapshot has been published to npm. Test the snapshot by intalling your package globally: pnpm i -g @shopify/cli@0.0.0-snapshot-20250122123941
|
Coverage report
Show files with reduced coverage 🔻
Test suite run success2001 tests passing in 904 suites. Report generated by 🧪jest coverage report action from 6121c3d |
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.
Would be best to test this with a snapshot release, to verify that it works after bundling
This reverts commit 2688080.
We detected some changes at packages/*/src and there are no updates in the .changeset. |
@gonzaloriestra @isaacroldan Reverted the changeset, and tested with this snapshot. The url generated is the following and it works, so it's finding the path correctly:
|
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!
WHY are these changes introduced?
Originally reported here.
Under some unknown circumstances, NX might complain about circular dependencies:
WHAT is this pull request doing?
Replace
require.resolve
withfindPathUp
to ensure Nx doesn't think@shopify/cli
is a dependency.How to test your changes?
shopify theme profile --url /
should still work.Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist