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

Jump to bottom

module: fix check for package.json at volume root #33476

Merged
merged 1 commit into from May 22, 2020
Merged

module: fix check for package.json at volume root #33476

merged 1 commit into from May 22, 2020

Conversation

GeoffreyBooth
Copy link
Member

Fixes #33438. A package.json at the volume root containing "type": "module" now behaves as documented. Thanks to @vitalets for pinpointing exactly where the problem was.

No idea how to write a test for this, but I tested it manually and this fixes the issue.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

cc @nodejs/modules-active-members

@GeoffreyBooth GeoffreyBooth added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 20, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31435/

guybedford
guybedford reviewed May 20, 2020
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we also have the esm resolver case to check too? Or is that already working?

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented May 20, 2020 •
edited

Note we also have the esm resolver case to check too? Or is that already working?

That seems to already be working. I did the test described in #33438 and got that to pass. I also tested one level down, where an index.cjs contained import('./test.js') and test.js contained import fs from 'fs', and that worked. Ditto when renaming index.cjs to index.js and then to index.mjs.

guybedford
guybedford approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, thanks.

@GeoffreyBooth GeoffreyBooth force-pushed the fix-package-json-type-at-root branch from e34cc24 to 036dfc1 Compare May 20, 2020 15:19
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31451/

@GeoffreyBooth GeoffreyBooth force-pushed the fix-package-json-type-at-root branch from 036dfc1 to cd2dc86 Compare May 20, 2020 19:23
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31457/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31458/

@GeoffreyBooth
Copy link
Member Author

https://ci.nodejs.org/job/node-test-pull-request/31458/ is green, dunno what's going on with the GitHub Actions.

@GeoffreyBooth GeoffreyBooth force-pushed the fix-package-json-type-at-root branch from cd2dc86 to a94a5ba Compare May 22, 2020 05:25
@GeoffreyBooth GeoffreyBooth merged commit a94a5ba into nodejs:master May 22, 2020
@GeoffreyBooth GeoffreyBooth deleted the fix-package-json-type-at-root branch May 22, 2020 05:27
@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented May 22, 2020 •
edited

Landed in 51af89f.

GeoffreyBooth added a commit that referenced this pull request May 22, 2020
@GeoffreyBooth
module: fix check for package.json at volume root
51af89f 
Fix package.json files at the volume root so that
when they contain {"type": "module"}, they behave
as documented, like such a package.json file in
any other folder.

Fixes: #33438

PR-URL: #33476
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@GeoffreyBooth @codebytere
module: fix check for package.json at volume root
78c5c4c 
Fix package.json files at the volume root so that
when they contain {"type": "module"}, they behave
as documented, like such a package.json file in
any other folder.

Fixes: #33438

PR-URL: #33476
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
@codebytere
Copy link
Member

@guybedford this caused a CITGM failure on v14.x - see #34093 (comment)

MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
@GeoffreyBooth @MylesBorins
module: fix check for package.json at volume root
3fd61ee 
Fix package.json files at the volume root so that
when they contain {"type": "module"}, they behave
as documented, like such a package.json file in
any other folder.

Fixes: #33438

PR-URL: #33476
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
Merged
@MylesBorins
Copy link
Member

Just got bit by this in prepping v14.6.0

I think we should revert this change as it is causing broad failures in various modules

MylesBorins added a commit to MylesBorins/node that referenced this pull request Jul 16, 2020
@MylesBorins
Revert "module: fix check for package.json at volume root"
50e7859 
This reverts commit 51af89f.

This has needed to be backed out of both the 14.5.0 and 14.6.0
releases due to creating regressions across multiple projects
including:

* coffeescript
* JSONStream
* gulp
* and more

We should reopen a PR to figure out how to land this in a way
that is non-breaking.

Refs: nodejs#33476
@MylesBorins MylesBorins mentioned this pull request Jul 16, 2020
Closed
MylesBorins added a commit that referenced this pull request Jul 20, 2020
@MylesBorins
Revert "module: fix check for package.json at volume root"
da95dd7 
This reverts commit 51af89f.

This has needed to be backed out of both the 14.5.0 and 14.6.0
releases due to creating regressions across multiple projects
including:

* coffeescript
* JSONStream
* gulp
* and more

We should reopen a PR to figure out how to land this in a way
that is non-breaking.

Refs: #33476

PR-URL: #34403
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins
Copy link
Member

This change has been reverted on master.

cjihrig pushed a commit that referenced this pull request Jul 23, 2020
@MylesBorins @cjihrig
Revert "module: fix check for package.json at volume root"
247ce2e 
This reverts commit 51af89f.

This has needed to be backed out of both the 14.5.0 and 14.6.0
releases due to creating regressions across multiple projects
including:

* coffeescript
* JSONStream
* gulp
* and more

We should reopen a PR to figure out how to land this in a way
that is non-breaking.

Refs: #33476

PR-URL: #34403
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@jkrems jkrems jkrems approved these changes

@guybedford guybedford guybedford approved these changes

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ES modules] package.json located in root path can't be resolved when checking type field
8 participants
@GeoffreyBooth @nodejs-github-bot @codebytere @MylesBorins @ljharb @jkrems @guybedford @richardlau