[v16.x backport] loader: return package format from defaultResolve if known by dygabo · Pull Request #41752 · nodejs/node · GitHub

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

Review requested:

@nodejs/modules

fyi @nodejs/loaders

Shouldn't 5fe75b0 come before c75d953?

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.

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

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

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

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

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

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

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

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

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

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

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>

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>

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 5fedf30 Compare 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-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022