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

deps: add deprecation warning for import assertion syntax #51631

Closed
wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 1, 2024

Not sure what the semverness of this would be, hopefully we can backport it to v18.x if #51136 can land there. Here's what the warning looks like:

$ ./node --input-type=module -e 'import "./test/fixtures/empty.js" assert {}'
(node:94657) V8: file://…/[eval1]:1 'assert' is deprecated in import statements and support will be removed in 12.6; use 'with' instead
(Use `node --trace-warnings ...` to show where the warning was created)
$ ./node --input-type=module -e 'import "./test/fixtures/empty.js" with {}' 
$ ./node -e 'import("./test/fixtures/empty.js", { assert:{} })'               
(node:94696) V8: [eval]:1 'assert' is deprecated in import statements and support will be removed in 12.6; use 'with' instead
(Use `node --trace-warnings ...` to show where the warning was created)
$ ./node -e 'import("./test/fixtures/empty.js", { with:{} })'  

Should we mutate the V8 warning to say "in a future version" instead of "in 12.6"?

Refs: #51622

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Feb 1, 2024
Original commit message:

    [import-attributes] Deprecate 'assert' for removal in 12.6

    See https://groups.google.com/a/chromium.org/g/blink-dev/c/ZHvzLaJZRvo/m/FgNDBjrtBQAJ

    Bug: v8:10958
    Change-Id: I4d21c9f7aad1024b198b4a1cdfb4792a011da464
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5055681
    Reviewed-by: Rezvan Mahdavi Hezaveh <[email protected]>
    Auto-Submit: Shu-yu Guo <[email protected]>
    Commit-Queue: Shu-yu Guo <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#92044}

Refs: v8/v8@ae5a4db
Original commit message:

    [import-attributes] Deprecate 'assert' for dynamic import as well

    Bug: v8:10958
    Change-Id: I7847bdb5d2c79f057f4e1df99f8f5889788f09cb
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5249778
    Commit-Queue: Shu-yu Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#92123}

Refs: v8/v8@26fd1df
@aduh95 aduh95 force-pushed the assert-deprecation-runtime-warning branch from d56a4be to 545a983 Compare February 1, 2024 13:12
@debadree25
Copy link
Member

Should we mutate the V8 warning to say "in a future version" instead of "in 12.6"?

I think "in a future version" or "12.6 of V8" would be better to use if its ok to float a patch for this or maybe a upstream v8 patch to say "12.6 of V8"

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 15, 2024

/cc @nodejs/v8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants