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

esm: import.meta.resolve exact module not found errors should return #49038

Merged

Conversation

guybedford
Copy link
Contributor

This resolves #49010, ensuring that not found errors to exact modules in import.meta.resolve still return the resolved string. Package not found errors remain module not found errors.

@nodejs/modules

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Aug 6, 2023
@guybedford
Copy link
Contributor Author

@JakobJingleheimer wondering if you have any clues as to what might be up with this asan test failure here?

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Aug 6, 2023

Looking at the output, it seems all cases are reporting okay, which makes me think there's an async issue (something is not waiting as it should). No idea why this would be isolated to ASan.

The test output appears to stop on:

Loader hooks → should handle a throwing top-level body → should handle empty plain object

Perhaps error here is not defined (or worse, null):

Ah, no, we know it's not because of switch (error?.code).

I would guess something is unexpectedly throwing, and that's getting swallowed (and ESM worker would be the most likely suspect for swallowing it as we experienced that A LOT during the off-thread PR).

I would still look here for the source of the unexpected throw

({ url } = error);
if (url) {
return url;
}
(and use that fs.writeSync trick to ensure debugging output actually happens).

@GeoffreyBooth
Copy link
Member

Looking at the output, it seems all cases are reporting okay, which makes me think there’s an async issue (something is not waiting as it should). No idea why this would be isolated to ASan.

https://openjs-foundation.slack.com/archives/C053UCCP940/p1690842970936839:

is there a reason the ASan build failed in ^? it did error in one of the ESM loader tests… so maybe it’s related… but the output doesn’t seem to show why 🤔 maybe there’s some non-determinism involved?

From July 31, so this might’ve been introduced by one of the earlier recent loaders PRs. cc @izaakschroeder

lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot

This comment was marked as duplicate.

@nodejs-github-bot

This comment was marked as duplicate.

@nodejs-github-bot
Copy link
Collaborator

@debadree25 debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Aug 12, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 13, 2023
@nodejs-github-bot nodejs-github-bot merged commit 569267d into nodejs:main Aug 13, 2023
30 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 569267d

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#49038
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #49038
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
PR-URL: nodejs#49038
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
rluvaton pushed a commit to rluvaton/node that referenced this pull request Aug 15, 2023
PR-URL: nodejs#49038
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023
PR-URL: #49038
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
PR-URL: #49038
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
PR-URL: nodejs#49038
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@targos targos added the backported-to-v18.x PRs backported to the v18.x-staging branch. label Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
PR-URL: #49038
Backport-PR-URL: #50669
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49038
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49038
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
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. backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import.meta.resolve non existing paths.
10 participants