Skip to content
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

Merged
merged 2 commits into from May 25, 2022

Conversation

notjosh
Copy link
Contributor

@notjosh notjosh commented Apr 26, 2022

Background

Hiya from Shop infra! 👋

A recent PR pulled @shopify/checkout-ui-extensions into Shop, and now we're getting warnings like the following:

(node:25986) [DEP0148] DeprecationWarning: Use of deprecated folder mapping "./" in the "exports" field module resolution of the package at /path/to/project/node_modules/@shopify/checkout-ui-extensions/package.json.
Update this package.json to use a subpath pattern like "./*".

It's not a problem unique to us - see other instances in major(ish) projects:

Solution

There are two common solutions:

  1. Replace "./": "./" with "./*": "./*"
  2. Add "./*": "./*" 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

mkdir /tmp/import-test
cd /tmp/import-test
yarn init -y
yarn add @shopify/checkout-ui-extensions
echo 'import("@shopify/checkout-ui-extensions/index.js");' > index.mjs

Existing functionality

$ node index.mjs
(node:29456) [DEP0148] DeprecationWarning: Use of deprecated folder mapping "./" in the "exports" field module resolution of the package at /private/tmp/test/node_modules/@shopify/checkout-ui-extensions/package.json imported from /private/tmp/test/index.mjs.
Update this package.json to use a subpath pattern like "./*".
(Use `node --trace-deprecation ...` to show where the warning was created)

Fixed functionality

Updating @shopify/checkout-ui-extensions without publishing can be mimicked by just updating node_modules/@shopify/checkout-ui-extensions/package.json to replace "./": "./" with "./*": "./*" (as this PR does)

$ node index.mjs
$ # no warning!

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@marvinhagemeister
Copy link
Contributor

ping @jamesvidler . Might be that this PR got lost in your notifications.

@@ -21,7 +21,7 @@
"import": "./index.mjs",
"require": "./index.js"
},
"./": "./"
"./*": "./*"
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Member

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.

@jamesvidler jamesvidler merged commit 53035db into main May 25, 2022
@notjosh notjosh deleted the chore/fix-deprecated-exports branch May 27, 2022 11:23
@shopify-shipit shopify-shipit bot temporarily deployed to production May 27, 2022 18:35 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants