FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bootstrap: polyfill Symbol.dispose/Symbol.asyncDispose with correct descriptor by ljharb · Pull Request #48703 · nodejs/node · GitHub

/ node Public

bootstrap: polyfill Symbol.dispose/Symbol.asyncDispose with correct descriptor - #48703

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
ljharb:symbol-descriptors
Jul 12, 2023
ConversationCommits (1)ChecksFiles changed
Merged

bootstrap: polyfill Symbol.dispose/Symbol.asyncDispose with correct descriptor#48703
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
ljharb:symbol-descriptors

Conversation

ljharb commented Jul 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Followup to #48518; fixes #48699

Happy to add a test if needed - but I both wasn't sure where it would go, and I also wasn't sure if this was worth testing since node rarely polyfills JS builtins and v8 will supply it soon enough anyways (backed by test262)

nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jul 8, 2023
ljharb mentioned this pull request Jul 8, 2023
Closed
ljharb force-pushed the symbol-descriptors branch from c379eaa to c08632c Compare July 8, 2023 19:03
ljharb force-pushed the symbol-descriptors branch 2 times, most recently from d8e640c to c70ae63 Compare July 8, 2023 19:09

ljharb commented Jul 9, 2023

Copy link
Copy Markdown
Member Author

I'm assuming this Mac OS test is flaky since it timed out - could someone rerun that one if needed?

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2023

nodejs-github-bot commented Jul 9, 2023

Copy link
Copy Markdown
Collaborator

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

benjamingr reviewed Jul 9, 2023
View reviewed changes
Comment thread lib/internal/process/pre_execution.js
// TODO(MoLow): Remove this polyfill once Symbol.dispose and Symbol.asyncDispose are available in V8.
// eslint-disable-next-line node-core/prefer-primordials
Symbol.dispose ??= SymbolDispose;
if (typeof Symbol.dispose !== 'symbol') {

benjamingr Jul 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We're guaranteed this runs before any userland code right?

(Just making sure this is safe since it's weird we set up Symbol.dispose here (if someone tampered with symbol they can set up a getter that throws here) but we use ObjectDefineProperty which is a primordial that guards against this sort of tampering))

ljharb Jul 9, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I’m assuming that the file called “pre execution” runs before any userland code - if not, this polyfill is highly problematic conceptually :-)

ljharb commented Jul 11, 2023

Copy link
Copy Markdown
Member Author

Are these 3 failing tests something I need to address?

nodejs-github-bot commented Jul 11, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow commented Jul 11, 2023

Copy link
Copy Markdown
Member

Are these 3 failing tests something I need to address?

seems like a fluke, I reran the tests

ljharb commented Jul 12, 2023

Copy link
Copy Markdown
Member Author

@MoLow still failing; are they flaky or do i need to look into it?

nodejs-github-bot commented Jul 12, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow commented Jul 12, 2023

Copy link
Copy Markdown
Member

@MoLow still failing; are they flaky or do i need to look into it?

passed now

MoLow added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 12, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 12, 2023
nodejs-github-bot merged commit c2c7260 into nodejs:main Jul 12, 2023

nodejs-github-bot commented Jul 12, 2023

Copy link
Copy Markdown
Collaborator

Landed in c2c7260

ljharb deleted the symbol-descriptors branch July 12, 2023 12:02
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
eb0aba5 
Followup to #48518; fixes #48699

PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
juanarbol mentioned this pull request Jul 13, 2023
Merged
github-actions Bot mentioned this pull request Jul 16, 2023
Open
23 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
8396660 
Followup to nodejs#48518; fixes nodejs#48699

PR-URL: nodejs#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
9c52644 
Followup to nodejs#48518; fixes nodejs#48699

PR-URL: nodejs#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

ruyadorno commented Sep 11, 2023

Copy link
Copy Markdown
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

ljharb commented Sep 11, 2023

Copy link
Copy Markdown
Member Author

@ruyadorno node 18 doesn't seem to have Symbol.dispose polyfilled, so i think this PR should have "do not backport" to <= 18?

SimenB commented Sep 19, 2023

Copy link
Copy Markdown
Member

node 18.18 was released today with the symbols, so maybe?

ljharb commented Sep 19, 2023

Copy link
Copy Markdown
Member Author

oof, ok then perhaps it will indeed backport cleanly into 18.18 :-)

targos pushed a commit that referenced this pull request Nov 27, 2023
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
e08fd98 
Followup to #48518; fixes #48699

PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos mentioned this pull request Nov 28, 2023
Merged
mhofman mentioned this pull request Dec 4, 2023
Closed
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
bootstrap: use correct descriptor for
8320281 
Followup to #48518; fixes #48699

PR-URL: nodejs/node#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
bootstrap: use correct descriptor for
80fb5b8 
Followup to #48518; fixes #48699

PR-URL: nodejs/node#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
MoLow MoLow approved these changes
+1 more reviewer
Linkgoron Linkgoron approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Symbol.{ dispose, asyncDispose } have incorrect descriptors

8 participants

Back | FazBrowse Home | New Git URL