-
Notifications
You must be signed in to change notification settings - Fork 37
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
Fix instances of deprecated folder mapping within exports in package.json #292
Conversation
ping @jamesvidler . Might be that this PR got lost in your notifications. |
@@ -21,7 +21,7 @@ | |||
"import": "./index.mjs", | |||
"require": "./index.js" | |||
}, | |||
"./": "./" | |||
"./*": "./*" |
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 checkout and post-purchase extensions, I believe we actually want to remove this condition altogether. We do not intend for you to be able to deeply import into the package.
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 this have any side-effects on the other packages in this PR if we make this change? I'm not familiar with how this folder mapping works.
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.
Since I already have a PR open for publishing new versions of our packages, i'll make the change to the checkout-ui-extensions packages in 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.
Actually, there's no harm in shipping this before the packages are published since i'll be merging with main
anyhow prior to publishing.
I don't have context on the other packages though in this PR. @lemonmade do you feel comfortable approving this?
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.
No, this mapping just determines what files are accessible to importers of the package. Conceivably, removing this could break consumers, if they are importing "deeply" from the package in a way we do not intend. I'm ok with breaking this use case, though.
Background
Hiya from Shop infra! 👋
A recent PR pulled
@shopify/checkout-ui-extensions
into Shop, and now we're getting warnings like the following:It's not a problem unique to us - see other instances in major(ish) projects:
Solution
There are two common solutions:
"./": "./"
with"./*": "./*"
"./*": "./*"
above"./": "./"
There was a window where solution 2 wouldn't work for some versions of Node 12 (ref), so some projects opted for solution 1.
I've opted for solution 2 simply because Node 12 is out of LTS in 4 days (and the latest versions of 12.x work fine for anyone still using Node 12), so I doubt it's going to have any meaningful impact.
🎩
The simplest way to test this is just setting up a small project:
Set up
Existing functionality
Fixed functionality
Updating
@shopify/checkout-ui-extensions
without publishing can be mimicked by just updatingnode_modules/@shopify/checkout-ui-extensions/package.json
to replace"./": "./"
with"./*": "./*"
(as this PR does)$ node index.mjs $ # no warning!
Checklist