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.x] Warning for import assertions #55873

Open
wants to merge 3 commits into
base: v18.x-staging
Choose a base branch
from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 15, 2024

Fixes: #55869

$ ./node --input-type=module -e 'import "data:application/json,{}" assert { type: "json" }'
(node:96221) V8: file:///Users/duhamean/Documents/node-v18.x/[eval1]: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)
$ echo $?
0
$ ./node --input-type=module -e 'import "data:application/json,{}" with { type: "json" }'
$ echo $?
0

@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. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. v8 engine Issues and PRs related to the V8 dependency. labels Nov 15, 2024
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@RedYetiDev RedYetiDev added backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Nov 16, 2024
@aduh95 aduh95 removed commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. labels Nov 16, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

aduh95 and others added 3 commits November 23, 2024 00:49
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]>
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]>
@aduh95 aduh95 force-pushed the warning-for-import-assertions branch from 5b2909a to 68d15a0 Compare November 22, 2024 23:51
@aduh95 aduh95 added the release-agenda Issues and PRs to discuss during the meetings of the Release team. label Nov 23, 2024
@aduh95
Copy link
Contributor Author

aduh95 commented Nov 23, 2024

Adding to the release agenda to make sure we have consensus for landing this. Worth noting that ded1535 is necessary to avoid a crash on the node-test-commit-arm-debug runner. I've tried reproducing locally but couldn't. Given the crash is limited to the debug build with an experimental syntax, we could deem it safe enough.

Test failure on ARM debug CI
not ok 2982 parallel/test-vm-module-link
  ---
  duration_ms: 910.48000
  severity: crashed
  exitcode: -5
  stack: |-
    (node:986098) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
    (Use `node --trace-warnings ...` to show where the warning was created)
    
    
    #
    # Fatal error in ../deps/v8/src/execution/execution.cc, line 368
    # Check failed: AllowJavascriptExecution::IsAllowed(isolate).
    #
    #
    #
    #FailureMessage Object: 0xfffff8c88ec8
     1: 0xaaaad5334178 node::DumpBacktrace(_IO_FILE*) [out/Debug/node]
     2: 0xaaaad54fc41c  [out/Debug/node]
     3: 0xaaaad54fc448  [out/Debug/node]
     4: 0xaaaad6fd28f8 V8_Fatal(char const*, int, char const*, ...) [out/Debug/node]
     5: 0xaaaad59e513c  [out/Debug/node]
     6: 0xaaaad59e618c v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [out/Debug/node]
     7: 0xaaaad574605c v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [out/Debug/node]
     8: 0xaaaad550fcac node::ProcessEmitWarningGeneric(node::Environment*, char const*, char const*, char const*) [out/Debug/node]
     9: 0xaaaad54350f8 node::errors::PerIsolateMessageListener(v8::Local<v8::Message>, v8::Local<v8::Value>) [out/Debug/node]
    10: 0xaaaad5a2ec00 v8::internal::MessageHandler::ReportMessageNoExceptions(v8::internal::Isolate*, v8::internal::MessageLocation const*, v8::internal::Handle<v8::internal::Object>, v8::Local<v8::Value>) [out/Debug/node]
    11: 0xaaaad5faf6d8 v8::internal::PendingCompilationErrorHandler::ReportWarnings(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Script>) const [out/Debug/node]
    12: 0xaaaad58afd20  [out/Debug/node]
    13: 0xaaaad58b0b6c  [out/Debug/node]
    14: 0xaaaad58b858c  [out/Debug/node]
    15: 0xaaaad58b8b54  [out/Debug/node]
    16: 0xaaaad58b8e54  [out/Debug/node]
    17: 0xaaaad58b9ba8 v8::internal::Compiler::GetSharedFunctionInfoForScript(v8::internal::Isolate*, v8::internal::Handle<v8::internal::String>, v8::internal::ScriptDetails const&, v8::ScriptCompiler::CompileOptions, v8::ScriptCompiler::NoCacheReason, v8::internal::NativesFlag) [out/Debug/node]
    18: 0xaaaad574ce3c  [out/Debug/node]
    19: 0xaaaad574d4a8 v8::ScriptCompiler::CompileModule(v8::Isolate*, v8::ScriptCompiler::Source*, v8::ScriptCompiler::CompileOptions, v8::ScriptCompiler::NoCacheReason) [out/Debug/node]
    20: 0xaaaad53ae9ec node::loader::ModuleWrap::New(v8::FunctionCallbackInfo<v8::Value> const&) [out/Debug/node]
    21: 0xaaaad57e25c4 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [out/Debug/node]
    22: 0xaaaad57e5f94  [out/Debug/node]
    23: 0xaaaad57e74e4 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [out/Debug/node]
    24: 0xaaaad66bd88c  [out/Debug/node]
  ...

Source: https://ci.nodejs.org/job/node-test-commit-arm-debug/15891/nodes=ubuntu2204_debug-arm64/consoleText

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. release-agenda Issues and PRs to discuss during the meetings of the Release team. v8 engine Issues and PRs related to the V8 dependency. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants