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

loader: fix package resolution for edge case #41218

Merged

Conversation

dygabo
Copy link
Contributor

@dygabo dygabo commented Dec 17, 2021

this commit solves a regression introduced with PR-40980.
if a resolve call results in a script with .mjs extension the format
is automatically set to module. This avoids the case where an additional
package.json in the same directory as the .mjs file would declare the
type to commonjs

the PR also contains a new test that covers this functionality.
It fixes the issue observed with #41167 during backport of #40980

relevant comment for this situation: #41167 (comment)

@nodejs/loaders

the issue was that for the case of yargs the type was solved wrongly to commonjs because of: this when importing yargs/helpers.
That happened because package.json had priority over the extension. With this PR the package.json information only applies if the extension of the script is .js

Refs: #40980
Refs: yargs/yargs#2068

@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. needs-ci PRs that need a full CI run. labels Dec 17, 2021
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
@dygabo dygabo changed the title loaders: fix package resolution for edge case loader: fix package resolution for edge case Dec 17, 2021
@tniessen
Copy link
Member

also cc @JakobJingleheimer

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2021
@nodejs-github-bot

This comment has been minimized.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can we add a test case where outer package.json has "type": "module" and inner package.json has "type": "commonjs" for completness? Also I'd like to see what happens when there is a query string or a hash is added to the "exports" url (e.g.: "exports": "./index.mjs?key=val"), when the file extension is .cjs, and when the file extension is something other (e.g.: .ts).

@dygabo
Copy link
Contributor Author

dygabo commented Dec 17, 2021

@aduh95 : please consider the rewrite based on your comments and on the existing implementation of defaultGetFormat as it covers also --experimental-specifier-resolution. : 6f7d871
I will prepare also some additional tests for the remaining cases

@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2021

I think we should go into the try block only if the extension is .js, otherwise we already know the format (or we know we can't know the format).
We should ensure the experimental warning is emitted only once.

@dygabo
Copy link
Contributor Author

dygabo commented Dec 17, 2021

pushed again based on review comments: 435dc9b. Please review.

we now have 6 test variations including those @aduh95 proposed.
one test scenario missing though: the one with a query string in the form "exports": "./index.mjs?key=val". Please provide an example and the expected behaviour in this context and I will prepare a test for that as well.

A real-world example using that feature would also be fine.

@dygabo dygabo requested a review from aduh95 December 17, 2021 16:01
@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2021

one test scenario missing though: the one with a query string in the form "exports": "./index.mjs?key=val". Please provide an example and the expected behaviour in this context and I will prepare a test for that as well.

I meant having the "exports" field in package.json equals to "./index.mjs?key=val" – so with a query string in the URL. It would also be interesting to have one with a hash string ("./index.mjs#key") and – why not – both ("./index.mjs?key=val#key"). The expected behavior would be for the query string and the hash part to be completely ignored to define the format value. Let me know if you need more info.

@JakobJingleheimer

This comment has been minimized.

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
@JakobJingleheimer

This comment has been minimized.

@aduh95

This comment has been minimized.

targos pushed a commit that referenced this pull request Jan 14, 2022
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>
@danielleadams
Copy link
Member

@dygabo this also didn't land cleanly on 16.x (with #40980) - do you mind backporting?

@dygabo
Copy link
Contributor Author

dygabo commented Jan 29, 2022

@danielleadams : also for this one I will look into it.

@Flarna
Copy link
Member

Flarna commented Jan 29, 2022

@dygabo I recommend a single backport PR for both as we either want both or nothing.

see here to get some hints how a backport PR should look like.

dygabo added a commit to dynatrace-oss-contrib/node that referenced this pull request Jan 29, 2022
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>
dygabo added a commit to dynatrace-oss-contrib/node that referenced this pull request Jan 29, 2022
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 added a commit to dynatrace-oss-contrib/node that referenced this pull request Jan 29, 2022
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 added a commit to dynatrace-oss-contrib/node that referenced this pull request Jan 31, 2022
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
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
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
Flarna pushed a commit to dynatrace-oss-contrib/node that referenced this pull request Jan 31, 2022
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
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
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 added a commit to dynatrace-oss-contrib/node that referenced this pull request Feb 1, 2022
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
danielleadams pushed a commit that referenced this pull request Feb 5, 2022
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 danielleadams mentioned this pull request Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet