| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
As I mentioned in #51622 (comment) I'm open to landing #51136 on v18.x. |
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
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.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
|
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?) |
Sorry, something went wrong.
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). |
Sorry, something went wrong.
Sorry, something went wrong.
|
@targos it does not seem I can rerun the GHA. |
Sorry, something went wrong.
|
It's probably too old to be re-runnable. |
Sorry, something went wrong.
|
Should I push an empty commit to retrigger it? |
Sorry, something went wrong.
|
let's see if the main CI passes first. |
Sorry, something went wrong.
|
Please push a fresh commit. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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