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.

Sign up for GitHub

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

[v16.x backport] loader: return package format from defaultResolve if known #41752

Closed
wants to merge 3 commits into from
Closed

[v16.x backport] loader: return package format from defaultResolve if known #41752

wants to merge 3 commits into from

Conversation

dygabo
Copy link
Contributor

@dygabo dygabo commented Jan 29, 2022

This is a backport PR for PR #40980 (cc: @danielleadams)
During merging I did one branch for PR #40980 and #41218 according to input on the backport request thread from @Flarna
Additionally I had to cherry-pick the commit of PR #41132 (@aduh95) as this contains a bugfix needed by #40980

Following commits have been backported:

for PR #40980: 85d4cd3
for PR #41132: e5c1fd7
for PR #41218: 34e3dd5

Since this is my first backport PR hopefully I got all the detail steps right for the tooling to work properly.

make -j4 test passes all the tests.
make lint as well

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. v16.x labels Jan 29, 2022
dygabo added a commit to dynatrace-oss-contrib/node that referenced this pull request Jan 29, 2022
@dygabo
loader: fix package resolution for edge case
5fedf30 
this commit solves a regression introduced with PR-40980.
if a resolve call results in a script with .mjs extension the
is automatically set to . This avoids the case where an additional
 in the same directory as the .mjs file would declare the
 to commonjs

PR-URL: nodejs#41218
Refs: nodejs#40980
Refs: yargs/yargs#2068
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#41752
@dygabo dygabo mentioned this pull request Jan 29, 2022
Merged
@Flarna Flarna added the loaders Issues and PRs related to ES module loaders label Jan 29, 2022
@Flarna
Copy link
Member

Flarna commented Jan 29, 2022

fyi @nodejs/loaders

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@aduh95
Copy link
Contributor

aduh95 commented Jan 29, 2022

Shouldn't 5fe75b0 come before c75d953?

dygabo added a commit to dynatrace-oss-contrib/node that referenced this pull request Jan 29, 2022
@dygabo
loader: fix package resolution for edge case
883c643 
this commit solves a regression introduced with PR-40980.
if a resolve call results in a script with .mjs extension the
is automatically set to . This avoids the case where an additional
 in the same directory as the .mjs file would declare the
 to commonjs

PR-URL: nodejs#41218
Refs: nodejs#40980
Refs: yargs/yargs#2068
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#41752
@dygabo
Copy link
Contributor Author

dygabo commented Jan 29, 2022

Shouldn't 5fe75b0 come before c75d953?

correct, done that. as they are disjunct, if these commits get squashed when merging this PR it shouldn't make a difference.

@aduh95
Copy link
Contributor

aduh95 commented Jan 29, 2022

They won't get squashed, each commit will be cherry-picked on the staging branch when this PR lands (equivalent to the rebase and merge GitHub feature).

@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 29, 2022
@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions bot mentioned this pull request Jan 30, 2022
Open
25 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@danielleadams danielleadams added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 30, 2022
@github-actions
Copy link
Contributor

Fast-track has been requested by @danielleadams. Please 👍 to approve.

@github-actions github-actions bot mentioned this pull request Jan 31, 2022
Open
28 tasks
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2022
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Feb 1, 2022
Open
31 tasks
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@dygabo @danielleadams
loader: return package format from defaultResolve if known
bf3148e 
This is a proposed modification of defaultResolve to return the package
format in case it has been found during package resolution.
The format will be returned as described in the documentation:
https://nodejs.org/api/esm.html#resolvespecifier-context-defaultresolve
There is one new unit test as well:
test/es-module/test-esm-resolve-type.js

PR-URL: #40980
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Backport-PR-URL: #41752
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@dygabo @danielleadams
loader: fix package resolution for edge case
acb09e8 
this commit solves a regression introduced with PR-40980.
if a resolve call results in a script with .mjs extension the
is automatically set to . This avoids the case where an additional
 in the same directory as the .mjs file would declare the
 to commonjs

PR-URL: #41218
Refs: #40980
Refs: yargs/yargs#2068
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #41752
@danielleadams
Copy link
Member

@dygabo @Flarna This looks good, but the PR still needs to be rebased from the base branch for the original conflicts. I tried to run the rebase myself, but I think I need write access on the fork. I'm going to go ahead and prepare the release and then I can add pull in the backport tomorrow.

aduh95 and others added 3 commits February 1, 2022 08:09
@aduh95 @dygabo
fs: accept URL as argument for fs.rm and fs.rmSync
3a9a260 
PR-URL: nodejs#41132
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: nodejs#41752
@dygabo
loader: return package format from defaultResolve if known
3e60925 
This is a proposed modification of defaultResolve to return the package
format in case it has been found during package resolution.
The format will be returned as described in the documentation:
https://nodejs.org/api/esm.html#resolvespecifier-context-defaultresolve
There is one new unit test as well:
test/es-module/test-esm-resolve-type.js

PR-URL: nodejs#40980
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Backport-PR-URL: nodejs#41752
@dygabo
loader: fix package resolution for edge case
29a7f7b 
this commit solves a regression introduced with PR-40980.
if a resolve call results in a script with .mjs extension the
is automatically set to . This avoids the case where an additional
 in the same directory as the .mjs file would declare the
 to commonjs

PR-URL: nodejs#41218
Refs: nodejs#40980
Refs: yargs/yargs#2068
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#41752
@dygabo
Copy link
Contributor Author

dygabo commented Feb 1, 2022

@dygabo @Flarna This looks good, but the PR still needs to be rebased from the base branch for the original conflicts. I tried to run the rebase myself, but I think I need write access on the fork. I'm going to go ahead and prepare the release and then I can add pull in the backport tomorrow.

@danielleadams: I rebased the branch now on current head of v16.x-staging. Please let me know if it's fine now.

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 1, 2022
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Feb 2, 2022
Open
38 tasks
@Flarna
Copy link
Member

Flarna commented Feb 2, 2022

@danielleadams Any chance that this gets included into the upcoming node 16 release? Otherwise I fear we will end up again in a rebase cycle in next release.

@danielleadams
Copy link
Member

@Flarna yes, definitely. I'll pull this one in. The 16.x release won't go out for another week.

This was referenced Feb 3, 2022
Open
Open
Open
danielleadams pushed a commit that referenced this pull request Feb 5, 2022
@aduh95 @danielleadams
fs: accept URL as argument for fs.rm and fs.rmSync
b671194 
PR-URL: #41132
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: #41752
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 5, 2022
@dygabo @danielleadams
loader: return package format from defaultResolve if known
694dc12 
This is a proposed modification of defaultResolve to return the package
format in case it has been found during package resolution.
The format will be returned as described in the documentation:
https://nodejs.org/api/esm.html#resolvespecifier-context-defaultresolve
There is one new unit test as well:
test/es-module/test-esm-resolve-type.js

PR-URL: #40980
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Backport-PR-URL: #41752
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 5, 2022
@dygabo @danielleadams
loader: fix package resolution for edge case
d422e58 
this commit solves a regression introduced with PR-40980.
if a resolve call results in a script with .mjs extension the
is automatically set to . This avoids the case where an additional
 in the same directory as the .mjs file would declare the
 to commonjs

PR-URL: #41218
Refs: #40980
Refs: yargs/yargs#2068
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #41752
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@danielleadams
Copy link
Member

Landed in 99a90db...d422e58

@github-actions github-actions bot mentioned this pull request Feb 6, 2022
Open
27 tasks
@dygabo dygabo deleted the backport-40980-to-v16.x branch February 10, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@danielleadams danielleadams danielleadams approved these changes

@aduh95 aduh95 aduh95 approved these changes

@Flarna Flarna Flarna approved these changes

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system. loaders Issues and PRs related to ES module loaders needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dygabo @nodejs-github-bot @Flarna @aduh95 @danielleadams @GeoffreyBooth