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

dygabo · 2022-01-29T16:38:09Z

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 · 2022-01-29T16:38:14Z

Review requested:

@nodejs/modules

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Backport-PR-URL: nodejs#41752

Flarna · 2022-01-29T17:42:48Z

fyi @nodejs/loaders

aduh95 · 2022-01-29T17:52:13Z

Shouldn't 5fe75b0 come before c75d953?

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Backport-PR-URL: nodejs#41752

dygabo · 2022-01-29T18:35:43Z

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 · 2022-01-29T18:38:55Z

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).

github-actions · 2022-01-30T23:03:15Z

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

nodejs-github-bot · 2022-01-31T21:59:57Z

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

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 <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Backport-PR-URL: #41752

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Backport-PR-URL: #41752

danielleadams · 2022-02-01T03:48:32Z

@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.

PR-URL: nodejs#41132 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Backport-PR-URL: nodejs#41752

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 <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Backport-PR-URL: nodejs#41752

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Backport-PR-URL: nodejs#41752

dygabo · 2022-02-01T07:28:51Z

@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.

nodejs-github-bot · 2022-02-01T15:51:58Z

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

Flarna · 2022-02-02T07:23:20Z

@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 · 2022-02-02T16:49:49Z

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

PR-URL: #41132 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Backport-PR-URL: #41752 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Danielle Adams <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

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 <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Backport-PR-URL: #41752 Reviewed-By: Danielle Adams <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Backport-PR-URL: #41752 Reviewed-By: Danielle Adams <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

danielleadams · 2022-02-05T23:04:26Z

Landed in 99a90db...d422e58

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 force-pushed the backport-40980-to-v16.x branch from 0cb30ba to 5fedf30Compare January 29, 2022 16:41

dygabo mentioned this pull request Jan 29, 2022
loader: return package format from defaultResolve if known #40980
Merged

Flarna added the loaders Issues and PRs related to ES module loaders label Jan 29, 2022

Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022