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: drop support for import assertions #52104

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Mar 15, 2024

Description for changelog: #52104 (comment)


This patch removes support for the assert keyword for import attributes. It was an old variant of the proposal that was only shipped in V8 and no other
engine, and that has then been replaced by the with keyword.

Chrome is planning to remove support for assert
in version 126, which will be released in June.

Node.js already supports the with keyword for
import attributes, and this patch does not change that.

Fixes #51622

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 15, 2024
@aduh95
Copy link
Contributor

aduh95 commented Mar 15, 2024

Worth noting that v18.x does not support with syntax at all, so currently anyone trying to write code that uses import assertions/attributes has to use the older assert syntax to write code that works in all release lines. If/when this PR lands, there will be no way to write code that works everywhere. That being said, both syntaxes are experimental and therefore not subject to semver.

@richardlau
Copy link
Member

Worth noting that v18.x does not support with syntax at all, so currently anyone trying to write code that uses import assertions/attributes has to use the older assert syntax to write code that works in all release lines. If/when this PR lands, there will be no way to write code that works everywhere. That being said, both syntaxes are experimental and therefore not subject to semver.

As I mentioned in #51622 (comment) I'm open to landing #51136 on v18.x.


await rejects(
import(`data:text/javascript,import${JSON.stringify(jsonModuleDataUrl)}assert{type:"json"}`),
SyntaxError
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This currently throws "unexpected token 'assert" -- I submitted a patch to V8 to make the error explicitly recommend to use 'with' instead: https://chromium-review.googlesource.com/c/v8/v8/+/5376260

This patch removes support for the `assert` keyword
for import attributes. It was an old variant of the
proposal that was only shipped in V8 and no other
engine, and that has then been replaced by the `with`
keyword.

Chrome is planning to remove support for `assert`
in version 126, which will be released in June.

Node.js already supports the `with` keyword for
import attributes, and this patch does not change that.
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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2024
@joyeecheung
Copy link
Member

joyeecheung commented Apr 17, 2024

Since V8 will remove assert and it’s stage 3 going back to stage 2 and then broken in the spec anyway, is it semver major?(To me it looks more like experimental features and not subject to semver and @aduh95 also said something similar in #52104 (comment) ). It would not be great to continue having assert lingering around in v22 and then V8 breaks it and we cannot upgrade V8 on v22.

If it’s semver-major we should try getting it into v22 in time. Otherwise we can land it later on v22 (still nicer to have it in the beginning but I guess it’s not the end of the world to land it in 22.1.0 or something).

cc @targos @nodejs/releasers

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2024
@nodejs-github-bot
Copy link
Collaborator

@nicolo-ribaudo
Copy link
Contributor Author

The CI failures don't seem related to my changes, but maybe I should try rebasing? Or could somebody re-trigger CI?

The arm build is failing both on windows and OSX.

For the test failures on Linux, one is a timeout and the other one is marked as flaky.

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

@RafaelGSS this should be in v22

@nicolo-ribaudo
Copy link
Contributor Author

nicolo-ribaudo commented Apr 18, 2024

Now the macOS failure is a flaky test (not ok 1443 parallel/test-http-server-headers-timeout-keepalive # TODO : Fix flaky test at https://ci.nodejs.org/job/node-test-commit-osx-arm/16770/nodes=osx11/consoleFull).

The failure on linux is a timeout:

09:37:39 not ok 483 async-hooks/test-writewrap
09:37:39   ---
09:37:39   duration_ms: 120041.78500
09:37:39   severity: fail
09:37:39   exitcode: -15
09:37:39   stack: |-
09:37:39     timeout

(out of curiosity, when you have to re-run CI due to flaky tests, do you re-run everything or just the failed jobs?)

@targos
Copy link
Member

targos commented Apr 18, 2024

out of curiosity, when you have to re-run CI due to flaky tests, do you re-run everything or just the failed jobs?

When we use the "Resume build" button on Jenkins, it reruns all jobs that are either orange (only flaky failures) or red (at least one non-flaky failure).

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

@targos it does not seem I can rerun the GHA.

@targos
Copy link
Member

targos commented Apr 18, 2024

It's probably too old to be re-runnable.

@nicolo-ribaudo
Copy link
Contributor Author

Should I push an empty commit to retrigger it?

@mcollina
Copy link
Member

let's see if the main CI passes first.

@mcollina
Copy link
Member

Please push a fresh commit.

LunarTwilight added a commit to WikiaUsers/ContentReviewLog that referenced this pull request May 15, 2024
subs1stem added a commit to rosteleset/SmartYard-Server that referenced this pull request May 20, 2024
royra referenced this pull request in livecycle/preevy May 28, 2024
- remove single usage of import assertion (import ... with/assert) which was breaking compat with older node 18 versions (see https://github.com/nodejs/node/pull/52104\#issuecomment-2000337367)
royra added a commit to livecycle/preevy that referenced this pull request May 28, 2024
- remove import `with` keyword, see #492. tried replacing with newer `assert` syntax but it was [breaking](nodejs/node#52104 (comment)) older node 18 versions.
- upgrade azure package to support node v22
- bump packaged oclif node version to latest LTS (20.13.1)
- bump node version for CI workflows to 20.x
- keep supported version in package.json to >=18.0.0

---------

Co-authored-by: Roy Razon <roy@livecycle.io>
valadaptive added a commit to valadaptive/scratchblocks that referenced this pull request Jun 16, 2024
These no longer build in Node 22 since import assertions were removed:
nodejs/node#52104
KockaAdmiralac pushed a commit to WikiaUsers/ContentReviewLog that referenced this pull request Jul 5, 2024
* fix syntax error due to `assert`

nodejs/node#52104 (comment)

* fix missed assert

thanks for pointing it out puxlit

* bump nodejs version and regenerate package-lock.json per kocka's request
nodejs-github-bot pushed a commit that referenced this pull request Nov 18, 2024
PR-URL: #55883
Refs: #52104
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#55883
Refs: nodejs#52104
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
PR-URL: nodejs#55883
Refs: nodejs#52104
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 added a commit that referenced this pull request Nov 26, 2024
PR-URL: #55883
Refs: #52104
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
GH-dragonwarrior added a commit to GH-dragonwarrior/preevy that referenced this pull request Dec 10, 2024
- remove import `with` keyword, see livecycle/preevy#492. tried replacing with newer `assert` syntax but it was [breaking](nodejs/node#52104 (comment)) older node 18 versions.
- upgrade azure package to support node v22
- bump packaged oclif node version to latest LTS (20.13.1)
- bump node version for CI workflows to 20.x
- keep supported version in package.json to >=18.0.0

---------

Co-authored-by: Roy Razon <roy@livecycle.io>
aduh95 added a commit that referenced this pull request Dec 10, 2024
PR-URL: #55883
Refs: #52104
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-flag assert syntax for import attributes in Node.js 22