GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — loader: fix package resolution for edge case by dygabo · Pull Request #41218 · nodejs/node

Conversation

@dygabo

@dygabo dygabo commented Dec 17, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Dec 17, 2021

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@dygabo dygabo mentioned this pull request Dec 17, 2021
v17.3.0 release proposal #41167
Merged
@dygabo
loader: fix package resolution for edge case
a149086 
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 force-pushed the fix-for-pr-40980 branch from cbd41ac to a149086 Compare December 17, 2021 12:02
@dygabo dygabo changed the title loaders: fix package resolution for edge case loader: fix package resolution for edge case Dec 17, 2021
@dygabo
fixup! loader: fix package resolution for edge case
db7a12a
@tniessen
tniessen requested review from Flarna, aduh95 and bmeck December 17, 2021 13:31
@tniessen

tniessen commented Dec 17, 2021

Copy link
Copy Markdown
Member

also cc @JakobJingleheimer
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 17, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 17, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes

@aduh95 aduh95 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes
Comment thread lib/internal/modules/esm/resolve.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@dygabo
rewrite based on review comments and defaultGetFormat implementation
6f7d871
@dygabo

dygabo commented Dec 17, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Dec 17, 2021

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dygabo
review comments, rework tests
435dc9b
@dygabo

dygabo commented Dec 17, 2021

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dygabo
dygabo requested a review from aduh95 December 17, 2021 16:01
aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes
Comment thread lib/internal/modules/esm/resolve.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Dec 17, 2021

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JakobJingleheimer

This comment has been minimized.

Sign in to view
aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes
Comment thread lib/internal/modules/esm/resolve.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/modules/esm/resolve.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@JakobJingleheimer

This comment has been minimized.

Sign in to view
@aduh95

This comment has been minimized.

Sign in to view
@dygabo
review comments, test cases added for query and hash export urls
f9fe067
This was referenced Dec 24, 2021
CI Reliability 2021-12-24 nodejs/reliability#154
Open
CI Reliability 2021-12-25 nodejs/reliability#155
Open
CI Reliability 2021-12-26 nodejs/reliability#156
Open
CI Reliability 2021-12-27 nodejs/reliability#157
Open
CI Reliability 2021-12-28 nodejs/reliability#158
Open
CI Reliability 2021-12-29 nodejs/reliability#159
Open
CI Reliability 2021-12-30 nodejs/reliability#160
Open
@github-actions github-actions Bot mentioned this pull request Dec 31, 2021
CI Reliability 2021-12-31 nodejs/reliability#161
Open
26 tasks
targos pushed a commit that referenced this pull request Jan 14, 2022
@dygabo @targos
loader: fix package resolution for edge case
68fd2ac 
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]>
@targos targos mentioned this pull request Jan 16, 2022
v17.4.0 release proposal #41557
Merged
@danielleadams

danielleadams commented Jan 29, 2022

Copy link
Copy Markdown
Contributor

@dygabo this also didn't land cleanly on 16.x (with #40980) - do you mind backporting?
dygabo reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@danielleadams danielleadams added the backport-requested-v16.x label Jan 29, 2022
@dygabo

dygabo commented Jan 29, 2022

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Flarna

Flarna commented Jan 29, 2022

Copy link
Copy Markdown
Member

@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 and danielleadams reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dygabo dygabo mentioned this pull request Jan 29, 2022
[v16.x backport] loader: return package format from defaultResolve if known #41752
Closed
@Flarna Flarna added the backport-open-v16.x label Jan 29, 2022
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 <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #41752
dygabo added a commit to dynatrace-oss-contrib/node that referenced this pull request Feb 1, 2022
@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 <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#41752
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 <[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 danielleadams added backported-to-v16.x and removed backport-requested-v16.x labels Feb 6, 2022
@danielleadams danielleadams mentioned this pull request Feb 6, 2022
v16.14.0 proposal #41804
Merged
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
@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes
@jasnell jasnell jasnell approved these changes
@guybedford guybedford guybedford approved these changes
@aduh95 aduh95 aduh95 approved these changes
@Flarna Flarna Awaiting requested review from Flarna
@bmeck bmeck Awaiting requested review from bmeck

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@dygabo @nodejs-github-bot @tniessen @aduh95 @JakobJingleheimer @iocumine @guybedford @Flarna @danielleadams @ljharb @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.