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] lib: move Symbol[Async]Dispose polyfills to internal/util #55264

Open
wants to merge 1 commit into
base: v20.x-staging
Choose a base branch
from
Open

[v20.x backport] lib: move Symbol[Async]Dispose polyfills to internal/util #55264

wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 4, 2024

Backport of #54853

@aduh95
lib: move Symbol[Async]Dispose polyfills to internal/util
8c960bc 
PR-URL: nodejs#54853
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http
  • @nodejs/http2
  • @nodejs/net
  • @nodejs/streams
  • @nodejs/test_runner

@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 4, 2024
@targos
Copy link
Member

targos commented Oct 4, 2024 •
edited
Loading

I take the opportunity to ask: how is it that when applied to v22.x-staging, cdae315 fails (because SymbolAsyncDispose is no longer in primordials per the change being backported here), while on main it's perfectly fine?
CleanShot 2024-10-04 at 19 25 09

node/lib/internal/quic/quic.js

Lines 19 to 21 in d2ad9b4

SymbolAsyncDispose,
Uint8Array,
} = primordials;

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 4, 2024

I'm not sure, I guess the test doesn't cover that method, or it's being inherited from somewhere else 🤔

@targos
Copy link
Member

targos commented Oct 5, 2024 •
edited
Loading

I've checked and primordials.SymbolAsyncDispose exists on main.

Opened #55276

mcollina
mcollina approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
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 @mcollina