bootstrap: polyfill Symbol.dispose/Symbol.asyncDispose with correct descriptor#48703
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added needs-ci 
ljharbforce-pushed the symbol-descriptors branch 2 times, most recently from d8e640c to c70ae63Compare
ljharb commented Jul 9, 2023
I'm assuming this Mac OS test is flaky since it timed out - could someone rerun that one if needed? |
github-actionsbot removed the request-ci 
nodejs-github-bot commented Jul 9, 2023
| // 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(typeofSymbol.dispose!=='symbol'){ |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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
Are these 3 failing tests something I need to address? |
nodejs-github-bot commented Jul 11, 2023
MoLow commented Jul 11, 2023
seems like a fluke, I reran the tests |
ljharb commented Jul 12, 2023
@MoLow still failing; are they flaky or do i need to look into it? |
nodejs-github-bot commented Jul 12, 2023
MoLow commented Jul 12, 2023
passed now |
MoLow added author ready nodejs-github-bot removed the commit-queue nodejs-github-bot commented Jul 12, 2023
Landed in c2c7260 |
Followup to #48518; fixes#48699 PR-URL: #48703 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Followup to nodejs#48518; fixesnodejs#48699 PR-URL: nodejs#48703 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Followup to nodejs#48518; fixesnodejs#48699 PR-URL: nodejs#48703 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
ruyadorno commented Sep 11, 2023
This commit does not land cleanly on |
ljharb commented Sep 11, 2023
@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
node 18.18 was released today with the symbols, so maybe? |
ljharb commented Sep 19, 2023
oof, ok then perhaps it will indeed backport cleanly into 18.18 :-) |
Followup to #48518; fixes#48699 PR-URL: #48703 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Followup to #48518; fixes #48699 PR-URL: nodejs/node#48703 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Followup to #48518; fixes #48699 PR-URL: nodejs/node#48703 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
8 participants
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)