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.

Sign up for GitHub

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

Jump to bottom

esm: rename error code related to import attributes #50181

Merged
merged 3 commits into from
Oct 18, 2023
Merged

esm: rename error code related to import attributes #50181

merged 3 commits into from
Oct 18, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 14, 2023

No description provided.

@aduh95 aduh95 added experimental Issues and PRs related to experimental features. esm Issues and PRs related to the ECMAScript Modules implementation. needs-citgm PRs that need a CITGM CI run. labels Oct 14, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels Oct 14, 2023
JakobJingleheimer
JakobJingleheimer reviewed Oct 14, 2023
View reviewed changes
doc/api/errors.md
removed: REPLACEME
-->

An import assertion has failed, preventing the specified module to be imported.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
An import assertion has failed, preventing the specified module to be imported.
An import assertion has failed, preventing the specified module being imported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to keep it as is, it's the wording used today in https://nodejs.org/api/errors.html#err_import_assertion_type_failed.

doc/api/errors.md
removed: REPLACEME
-->

An import assertion is missing, preventing the specified module to be imported.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
An import assertion is missing, preventing the specified module to be imported.
An import assertion is missing, preventing the specified module being imported.

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 16, 2023 •
edited
Loading

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3333/

Test failures seems to match the ones happening on 21.0.0 release proposal, the only addition is torrent-stream but that's a timeout.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 16, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Oct 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit 37d4f08 into nodejs:main Oct 18, 2023
34 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 37d4f08

targos pushed a commit that referenced this pull request Oct 23, 2023
@aduh95 @targos
esm: rename error code related to import attributes
7740bf8 
PR-URL: #50181
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
@targos targos mentioned this pull request Oct 23, 2023
Merged
@aduh95 aduh95 deleted the esm-import-attributes branch October 24, 2023 10:19
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@aduh95 @alexfernandez
esm: rename error code related to import attributes
c374ab8 
PR-URL: nodejs#50181
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
@targos
Copy link
Member

targos commented Dec 13, 2023

Why is this dont-land-on-v20.x ? I need it for #50703

This was referenced Dec 13, 2023
Merged
Closed
targos pushed a commit to targos/node that referenced this pull request Dec 13, 2023
@aduh95 @targos
esm: rename error code related to import attributes
20efdc9 
PR-URL: nodejs#50181
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 13, 2023

I'm pretty sure I added the dont-land with the assumption that import attributes would not be backported.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 13, 2023

Ah no, I remember now, this PR changes the error codes, and that could break existing code.

@targos
Copy link
Member

targos commented Dec 14, 2023

Aren't we allowed to break code since this is an experimental feature?

@targos
Copy link
Member

targos commented Dec 14, 2023 •
edited
Loading

The thing is that a side-effect of #50703 is to change the error code for existing tests (that are updated by this PR).

Forget it. We can keep this out of LTS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakobJingleheimer JakobJingleheimer JakobJingleheimer left review comments

@himself65 himself65 himself65 approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aduh95 @nodejs-github-bot @targos @GeoffreyBooth @JakobJingleheimer @himself65