FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

esm: import.meta.resolve exact module not found errors should return by guybedford · Pull Request #49038 · nodejs/node · GitHub

/ node Public

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

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
guybedford:import-meta-resolve-not-found-fix
Aug 13, 2023
Merged

esm: import.meta.resolve exact module not found errors should return#49038
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
guybedford:import-meta-resolve-not-found-fix

Conversation

Copy link
Copy Markdown
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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

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

Copy link
Copy Markdown
Contributor Author

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

JakobJingleheimer commented Aug 6, 2023
edited
Loading

Copy link
Copy Markdown
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.

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

Copy link
Copy Markdown
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

Comment thread lib/internal/errors.js Outdated
Comment thread lib/internal/errors.js Outdated

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

This comment was marked as duplicate.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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 outstanding review comments, 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. 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


Back | FazBrowse Home | New Git URL