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: add a runtime warning when using import assertions #46901

Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 1, 2023

Refs: #46830

/cc @nodejs/loaders

@aduh95 aduh95 added the experimental Issues and PRs related to experimental features. label Mar 1, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @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 Mar 1, 2023
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

I think we should wait for a response from tc39/proposal-import-attributes#127 (comment) before landing this.

Comment on lines +64 to +65
'Import assertions are not a stable feature of the JavaScript language, ' +
'avoid relying on their current behavior and syntax as those might change ' +
Copy link
Member

Choose a reason for hiding this comment

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

Comma splice.

Suggested change
'Import assertions are not a stable feature of the JavaScript language, ' +
'avoid relying on their current behavior and syntax as those might change ' +
'Import assertions are not a stable feature of the JavaScript language. ' +
'Avoid relying on their current behavior and syntax as those might change ' +

@ljharb
Copy link
Member

ljharb commented Mar 1, 2023

It wouldn't be possible for TC39 to provide any timing guarantees - the proposal champions may be able to promise they're ready to present in March, but nobody can predict consensus ahead of time.

Multiple delegates have expressed that "assert" syntax is inappropriate for something that's not an assertion anymore, and the web can't use the assertion semantics, so the semantics will change - which suggests to me that the syntax is likely to as well, regardless of whether Chrome decides to unship the current syntax or not.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@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. commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2023
@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added dont-land-on-v14.x lts-watch-v18.x PRs that may need to be released in v18.x. labels Mar 2, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 3, 2023
@nodejs-github-bot nodejs-github-bot merged commit 5a7491b into nodejs:main Mar 3, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 5a7491b

@GeoffreyBooth
Copy link
Member

@aduh95 This landed without #46901 (comment) being addressed? The current text is incorrect grammar.

@aduh95 aduh95 deleted the import-assertion-runtime-warning branch March 6, 2023 10:44
aduh95 added a commit to aduh95/node that referenced this pull request Mar 6, 2023
nodejs-github-bot pushed a commit that referenced this pull request Mar 7, 2023
Refs: #46901 (comment)
PR-URL: #46971
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Mar 13, 2023
PR-URL: #46901
Refs: #46830
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
targos pushed a commit that referenced this pull request Mar 13, 2023
Refs: #46901 (comment)
PR-URL: #46971
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Mar 14, 2023
PR-URL: #46901
Refs: #46830
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
targos pushed a commit that referenced this pull request Mar 14, 2023
Refs: #46901 (comment)
PR-URL: #46971
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
PR-URL: #46901
Refs: #46830
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
Refs: #46901 (comment)
PR-URL: #46971
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed lts-watch-v18.x PRs that may need to be released in v18.x. labels Oct 27, 2023
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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants