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.

Sign up for GitHub

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

Jump to bottom

[v20.x backport] esm: use import attributes instead of import assertions #50183

Closed
wants to merge 6 commits into from
Closed

[v20.x backport] esm: use import attributes instead of import assertions #50183

wants to merge 6 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 14, 2023 •
edited
Loading

Backport of #50140

Because the version of V8 does not support with keyword, I have not ported the changes in the tests and the docs are still using assert. Actually, the V8 changes were not too difficult to backport, so I decided to do that. That way, with keyword will be supported on all non-EOL as soon as v18.x reaches EOL.

To avoid conflicts, I'm also backporting:

GeoffreyBooth and others added 2 commits October 14, 2023 14:15
@GeoffreyBooth @aduh95
module, esm: jsdoc for modules files
39f107f 
PR-URL: nodejs#49523
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@aduh95
esm: fix cache collision on JSON files using file: URL
9577a00 
PR-URL: nodejs#49887
Fixes: nodejs#49724
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/net
  • @nodejs/vm

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. labels Oct 14, 2023
@aduh95 aduh95 force-pushed the esm-import-attributes-v20 branch 2 times, most recently from d9602c7 to 7ae7c89 Compare October 14, 2023 14:28
@GeoffreyBooth GeoffreyBooth added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Oct 14, 2023
@aduh95
Copy link
Contributor Author

aduh95 commented Oct 22, 2023

@nodejs/v8 I would love if I could get a review of 17ab6e9 – I don't expect it to be dangerous, but since 20.x is about to be LTS I'd rather have a few extra pairs of eyes on it.

@targos
Copy link
Member

targos commented Oct 24, 2023

Looking at https://bugs.chromium.org/p/v8/issues/detail?id=13856:

@aduh95
deps: V8: cherry-pick a0fd3209dda8
0100c7b 
Original commit message:

    [import-attributes] Implement import attributes, with `assert` fallback

    In the past six months, the old import assertions proposal has been
    renamed to "import attributes" with the follwing major changes:
    1. the keyword is now `with` instead of `assert`
    2. unknown assertions cause an error rather than being ignored

    To preserve backward compatibility with existing applications that use
    `assert`, implementations _can_ keep it around as a fallback for both
    the static and dynamic forms.

    Additionally, the proposal has some minor changes that came up during
    the stage 3 reviews:
    3. dynamic import first reads all the attributes, and then verifies
       that they are all strings
    4. there is no need for a `[no LineTerminator here]` restriction before
       the `with` keyword
    5. static import syntax allows any `LiteralPropertyName` as attribute
       keys, to align with every other syntax using key-value pairs

    The new syntax is enabled by a new `--harmony-import-attributes` flag,
    disabled by default. However, the new behavioral changes also apply to
    the old syntax that is under the `--harmony-import-assertions` flag.

    This patch does implements (1), (3), (4) and (5). Handling of unknown
    import assertions was not implemented directly in V8, but delegated
    to embedders. As such, it will be implemented separately in d8 and
    Chromium.

    To simplify the review, this patch doesn't migrate usage of the term
    "assertions" to "attributes". There are many variables and internal
    functions that could be easily renamed as soon as this patch landes.
    There is one usage in the public API
    (`ModuleRequest::GetImportAssertions`) that will probably need to be
    aliased and then removed following the same process as for other API
    breaking changes.

    Bug: v8:13856
    Change-Id: I78b167348d898887332c5ca7468bc5d58cd9b1ca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4632799
    Commit-Queue: Shu-yu Guo <syg@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89110}

Refs: v8/v8@159c82c
Refs: v8/v8@a0fd320
@aduh95
deps: V8: cherry-pick ea996ad04a68
ecb8bec 
Original commit message:

    [import-attributes] Remove support for numeric keys

    During the 2023-09 TC39 meeting the proposal has been updated to remove support
    for bigint and float literals as import attribute keys, due to implementation
    difficulties in other engines and minimal added value for JS developers.

    GH issue: tc39/proposal-import-attributes#145

    Bug: v8:13856
    Change-Id: I0ede2bb10d6ca338a4b0870a1261ccbcd088c16f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4899760
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Cr-Commit-Position: refs/heads/main@{#90318}

Refs: v8/v8@ea996ad
@aduh95
esm: use import attributes instead of import assertions
d22b640 
The old import assertions proposal has been
renamed to "import attributes" with the follwing major changes:

1. The keyword is now `with` instead of `assert`.
2. Unknown assertions cause an error rather than being ignored,

This commit updates the documentation to encourage folks to use the new
syntax, and add aliases for module customization hooks.

PR-URL: nodejs#50140
Fixes: nodejs#50134
Refs: v8/v8@159c82c
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@aduh95
vm: use import attributes instead of import assertions
edc1c4e 
The old import assertions proposal has been
renamed to "import attributes" with the following major changes:

1. The keyword is now `with` instead of `assert`.
2. Unknown assertions cause an error rather than being ignored.

This PR updates the documentation to encourage folks to use the new
syntax, and add aliases to preserve backward compatibility.

PR-URL: nodejs#50141
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
@aduh95
Copy link
Contributor Author

aduh95 commented Oct 24, 2023

Thanks for the pointer! IIUC d22b640 already implements all the relevant changes.

@joyeecheung joyeecheung mentioned this pull request Nov 11, 2023
Closed
@targos
Copy link
Member

targos commented Nov 11, 2023

Landed in 27e957c...57efd52

targos pushed a commit that referenced this pull request Nov 11, 2023
@aduh95 @targos
deps: V8: cherry-pick a0fd3209dda8
16fd730 
Original commit message:

    [import-attributes] Implement import attributes, with `assert` fallback

    In the past six months, the old import assertions proposal has been
    renamed to "import attributes" with the follwing major changes:
    1. the keyword is now `with` instead of `assert`
    2. unknown assertions cause an error rather than being ignored

    To preserve backward compatibility with existing applications that use
    `assert`, implementations _can_ keep it around as a fallback for both
    the static and dynamic forms.

    Additionally, the proposal has some minor changes that came up during
    the stage 3 reviews:
    3. dynamic import first reads all the attributes, and then verifies
       that they are all strings
    4. there is no need for a `[no LineTerminator here]` restriction before
       the `with` keyword
    5. static import syntax allows any `LiteralPropertyName` as attribute
       keys, to align with every other syntax using key-value pairs

    The new syntax is enabled by a new `--harmony-import-attributes` flag,
    disabled by default. However, the new behavioral changes also apply to
    the old syntax that is under the `--harmony-import-assertions` flag.

    This patch does implements (1), (3), (4) and (5). Handling of unknown
    import assertions was not implemented directly in V8, but delegated
    to embedders. As such, it will be implemented separately in d8 and
    Chromium.

    To simplify the review, this patch doesn't migrate usage of the term
    "assertions" to "attributes". There are many variables and internal
    functions that could be easily renamed as soon as this patch landes.
    There is one usage in the public API
    (`ModuleRequest::GetImportAssertions`) that will probably need to be
    aliased and then removed following the same process as for other API
    breaking changes.

    Bug: v8:13856
    Change-Id: I78b167348d898887332c5ca7468bc5d58cd9b1ca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4632799
    Commit-Queue: Shu-yu Guo <syg@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89110}

Refs: v8/v8@159c82c
Refs: v8/v8@a0fd320
PR-URL: #50183
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@targos targos closed this Nov 11, 2023
targos pushed a commit that referenced this pull request Nov 11, 2023
@aduh95 @targos
deps: V8: cherry-pick ea996ad04a68
6547bd2 
Original commit message:

    [import-attributes] Remove support for numeric keys

    During the 2023-09 TC39 meeting the proposal has been updated to remove support
    for bigint and float literals as import attribute keys, due to implementation
    difficulties in other engines and minimal added value for JS developers.

    GH issue: tc39/proposal-import-attributes#145

    Bug: v8:13856
    Change-Id: I0ede2bb10d6ca338a4b0870a1261ccbcd088c16f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4899760
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Cr-Commit-Position: refs/heads/main@{#90318}

Refs: v8/v8@ea996ad
PR-URL: #50183
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
This was referenced Nov 12, 2023
Merged
Closed
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
Merged
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Dec 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

Assignees
No one assigned
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aduh95 @nodejs-github-bot @targos @GeoffreyBooth