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

esm: add a runtime warning when using import assertions by aduh95 · Pull Request #46901 · nodejs/node · GitHub

/ node Public

esm: add a runtime warning when using import assertions - #46901

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
aduh95:import-assertion-runtime-warning
Mar 3, 2023
ConversationCommits (1)ChecksFiles changed
Merged

esm: add a runtime warning when using import assertions#46901
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
aduh95:import-assertion-runtime-warning

Conversation

aduh95 commented Mar 1, 2023

Copy link
Copy Markdown
Contributor

Refs: #46830

/cc @nodejs/loaders

aduh95 added the experimental Issues and PRs related to experimental features. label Mar 1, 2023

nodejs-github-bot commented Mar 1, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

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
GeoffreyBooth reviewed Mar 1, 2023
View reviewed changes

GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/internal/modules/esm/assert.js
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 ' +

GeoffreyBooth Mar 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Mar 1, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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.

mcollina approved these changes Mar 2, 2023
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

mcollina approved these changes Mar 2, 2023
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, 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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2023

nodejs-github-bot commented Mar 2, 2023

Copy link
Copy Markdown
Collaborator

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

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 merged commit 5a7491b into nodejs:main Mar 3, 2023

nodejs-github-bot commented Mar 3, 2023

Copy link
Copy Markdown
Collaborator

Landed in 5a7491b

GeoffreyBooth commented Mar 3, 2023

Copy link
Copy Markdown
Member

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

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
esm: fix import assertion warning
f16b38e 
Refs: nodejs#46901 (comment)
aduh95 mentioned this pull request Mar 6, 2023
Merged
nodejs-github-bot pushed a commit that referenced this pull request Mar 7, 2023
esm: fix import assertion warning
0c4f8f2 
Refs: #46901 (comment)
PR-URL: #46971
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Mar 13, 2023
esm: add a runtime warning when using import assertions
3644d9d 
PR-URL: #46901
Refs: #46830
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
targos pushed a commit that referenced this pull request Mar 13, 2023
esm: fix import assertion warning
40f13eb 
Refs: #46901 (comment)
PR-URL: #46971
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
esm: add a runtime warning when using import assertions
2c621d6 
PR-URL: #46901
Refs: #46830
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
targos pushed a commit that referenced this pull request Mar 14, 2023
esm: fix import assertion warning
9d4d916 
Refs: #46901 (comment)
PR-URL: #46971
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos mentioned this pull request Mar 14, 2023
Merged
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
esm: add a runtime warning when using import assertions
96a39d1 
PR-URL: #46901
Refs: #46830
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
esm: fix import assertion warning
5b94e2b 
Refs: #46901 (comment)
PR-URL: #46971
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams mentioned this pull request Apr 11, 2023
Merged
richardlau mentioned this pull request Aug 24, 2023
Closed
privatenumber mentioned this pull request Sep 2, 2023
Closed
1 task
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

GeoffreyBooth GeoffreyBooth left review comments
ljharb ljharb approved these changes
mcollina mcollina approved these changes
jasnell jasnell approved these changes
JakobJingleheimer JakobJingleheimer approved these changes
+1 more reviewer
VoltrexKeyva VoltrexKeyva approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL