Reorder exports map #2148
Reorder exports map #2148
Conversation
heathernfran
force-pushed
the
exports-fields
branch
from
f85faa1 to
d6d1639
Compare
There was a problem hiding this comment.
Changes to package.json files look good. I think this needs a rebase and an adjustment of the changelogs as @vsumner has done a release this morning.
The tests doesn't seem like they're testing what we'd hope, see inline comments
| }); | ||
|
|
||
| it('specifies esnext as the first expected export', () => { | ||
| expect(packageJSON.exports['.'].esnext).toBe( |
There was a problem hiding this comment.
This seems like it asserts the value of a specific key instead of anything to do with the ordering. It also seems to only check the . export key rather than all keys.
I would expect that something like the following would cause this test to fail, but right now I think this would pass.
exports: {
"./foo" : { 'require': './foo.js', esnext: './foo.esnext' }
}
There was a problem hiding this comment.
I think this test is no longer needed, since the other test now includes checking that esnext is first.
| @@ -139,6 +139,18 @@ packages.forEach( | |||
| it('specifies the expected types', () => { | |||
| expect(packageJSON.types).toBe(expectedPackageJSON.types); | |||
| }); | |||
|
|
|||
| it('specifies the expected exports', () => { | |||
There was a problem hiding this comment.
It is possible for packages to have an exports key that does not contain "." - e.g. graphql-persisted, so I don't think this test is correct.
There was a problem hiding this comment.
Good catch, updated the test to check for ordered keys in exports map 👍
There was a problem hiding this comment.
Thanks for this @heathernfran. Just need to address @BPScott testing comments.
heathernfran
force-pushed
the
exports-fields
branch
from
97b72f4 to
25c2d23
Compare
heathernfran
force-pushed
the
exports-fields
branch
from
25c2d23 to
3097217
Compare
| @@ -9,6 +9,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). | |||
|
|
|||
| ### Changed | |||
|
|
|||
| - Reorder exports map to prioritize the `esnext` condition. [[#2148](https://github.com/Shopify/quilt/pull/2148)] | |||
| - Clean test output. [[#2091](https://github.com/Shopify/quilt/pull/2091)] | |||
There was a problem hiding this comment.
This hasn't been released, right?
There was a problem hiding this comment.
Looks like it hasn't - the last change was just in test files, which are ignored when determining what to release
| - Updated puppeteer to 13.1.3 ([#2149](https://github.com/Shopify/quilt/pull/2149)) | ||
| - Reorder exports map to prioritize the `esnext` condition. [[#2148](https://github.com/Shopify/quilt/pull/2148)] |
There was a problem hiding this comment.
Fix from earlier merge conflict
| @@ -139,6 +139,18 @@ packages.forEach( | |||
| it('specifies the expected types', () => { | |||
| expect(packageJSON.types).toBe(expectedPackageJSON.types); | |||
| }); | |||
|
|
|||
| it('specifies esnext, import, and require as the ordered keys in the exports map', () => { | |||
Description
https://github.com/Shopify/web/issues/56636
Webpack 5 can resolve exports from package.json. Reordering the exports map ensures that when libraries ship with only commonjs, esm compiling will take precedence.
Type of change
Checklist