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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 4, 2024 •
edited
Loading

Backport of #54853 and #55318

@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

@marco-ippolito marco-ippolito 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
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63571/

@aduh95
lib: move Symbol[Async]Dispose polyfills to internal/util
d6d91f2 
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]>
@aduh95 aduh95 force-pushed the symbol-async-dispose branch from 8c960bc to d6d91f2 Compare November 16, 2024 11:06
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63573/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63582/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63604/

marco-ippolito pushed a commit that referenced this pull request Nov 18, 2024
@aduh95 @marco-ippolito
lib: move Symbol[Async]Dispose polyfills to internal/util
00a2fc7 
PR-URL: #54853
Backport-PR-URL: #55264
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]>
marco-ippolito pushed a commit that referenced this pull request Nov 18, 2024
@aduh95 @marco-ippolito
tools: add polyfilled option to prefer-primordials rule
0b3fb34 
PR-URL: #55318
Backport-PR-URL: #55264
Reviewed-By: Michaël Zasso <[email protected]>
@marco-ippolito
Copy link
Member

Landed in f25a5b6...0b3fb34

@aduh95 aduh95 deleted the symbol-async-dispose branch November 18, 2024 09:46
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.
5 participants
@aduh95 @nodejs-github-bot @targos @marco-ippolito @mcollina