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

[v18.20.5 / next v20.x LTS version] NodeJS should provide some kind of warning when using import assertions as they are non-standard #55869

Closed
alexsch01 opened this issue Nov 15, 2024 · 11 comments
Labels
v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.

Comments

@alexsch01
Copy link
Contributor

alexsch01 commented Nov 15, 2024

Clarification

v18.20.4 (previous v18.x release) and v20.18.0 have a warning for both import assertions and import attributes

v18.20.5 removes this warning for both cases, I believe the import assertion warning should still be around

Seems to be caused by #55333

Version

v18.20.5

Platform

All

Subsystem

No response

What steps will reproduce the bug?

node --input-type=module -e 'import "data:application/json,{}" assert { type: "json" }'

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior? Why is that the expected behavior?

Regarding [What steps will reproduce the bug?]

It should provide some kind of warning since import assertions (not import attributes) are non-standard

Additional information

This also applies to the next v20.x LTS version

@alexsch01 alexsch01 changed the title [v18.20.5] NodeJS should provide some kind of warning when using import assertions as they are non-standard [v18.20.5 / future v20.x LTS version] NodeJS should provide some kind of warning when using import assertions as they are non-standard Nov 15, 2024
@alexsch01 alexsch01 changed the title [v18.20.5 / future v20.x LTS version] NodeJS should provide some kind of warning when using import assertions as they are non-standard [v18.20.5 / next v20.x LTS version] NodeJS should provide some kind of warning when using import assertions as they are non-standard Nov 15, 2024
@avivkeller
Copy link
Member

Adding a warning is a breaking change, and LTS lines are meant to be non-breaking.

@alexsch01
Copy link
Contributor Author

@redyetidev I should have clarified

v18.20.4 (previous v18.x release) and v20.18.0 have a warning for both import assertions and import attributes

v18.20.5 removes this warning for both cases, I believe the import assertion warning should still be around

@avivkeller avivkeller added the v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. label Nov 15, 2024
@avivkeller
Copy link
Member

avivkeller commented Nov 15, 2024

Oh, okay. The removal / addition of a warning is typically semver major, I wonder why it was backported.

@alexsch01
Copy link
Contributor Author

This is probably the cause #55333

@avivkeller
Copy link
Member

Yes, likely.

@nicolo-ribaudo
Copy link
Contributor

This is a bit difficult to fix, because V8 doesn't tell us which syntax is being used. Does any of the maintainers have any suggestion?

@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2024

We need to reconsider #51631

@avivkeller avivkeller added the v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. label Nov 15, 2024
@alexsch01
Copy link
Contributor Author

@aduh95 thanks for the PR, just letting you know that a v20.x version will be needed

@aduh95
Copy link
Contributor

aduh95 commented Nov 16, 2024

@alexsch01 I'm not sure why you ping me to repeat an information that's already in the OP. If you need a 20.x backport, make the PR yourself, or pay someone to do it for you (you can find support links in the README).

@alexsch01
Copy link
Contributor Author

my apologies

@avivkeller avivkeller removed the v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. label Nov 16, 2024
@aduh95 aduh95 added the v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. label Nov 16, 2024
@alexsch01
Copy link
Contributor Author

alexsch01 commented Feb 12, 2025

Fixed in v20.18.3 thank you aduh95!

[data.json]

{"abc":"def"}

[script_with.mjs]

import data from './data.json' with {type: 'json'}

console.log(process.version)
console.log(data)

running in node

v20.18.3
{ abc: 'def' }

[script_assert.mjs]

import data from './data.json' assert {type: 'json'}

console.log(process.version)
console.log(data)

running in node

(node:1092) V8: file:///somelocation/script_assert.mjs:1 'assert' is deprecated in import statements and support will be removed in a future version; use 'with' instead
(Use `node --trace-warnings ...` to show where the warning was created)
v20.18.3
{ abc: 'def' }

aduh95 added a commit to aduh95/node that referenced this issue Feb 13, 2025
aduh95 added a commit to aduh95/node that referenced this issue Feb 13, 2025
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
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: nodejs#55873
Fixes: nodejs#55869
Reviewed-By: Richard Lau <[email protected]>
aduh95 added a commit to aduh95/node that referenced this issue Feb 13, 2025
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
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: nodejs#55873
Fixes: nodejs#55869
Reviewed-By: Richard Lau <[email protected]>
aduh95 added a commit that referenced this issue Feb 13, 2025
aduh95 added a commit that referenced this issue Feb 13, 2025
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
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #55873
Fixes: #55869
Reviewed-By: Richard Lau <[email protected]>
aduh95 added a commit that referenced this issue Feb 13, 2025
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
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #55873
Fixes: #55869
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.
Projects
None yet
Development

No branches or pull requests

4 participants