lib: add %TypedArray% abstract constructor to primordials #36016

ExE-Boss · 2020-11-07T07:44:13Z

Refs: #35448
Refs: #36003
Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Supersedes and closes #32127

review?(@aduh95, @targos)

aduh95

Thanks a lot for doing this! I like it way more than my hack in the other PR. Two comments:

lib/internal/per_context/primordials.js

benjamingr

With Antoine's comments addressed :]

lib/internal/per_context/primordials.js

aduh95 · 2020-11-07T23:29:21Z

nit: you can also replace two instances in freeze_intrinsics.js:

--- a/lib/internal/freeze_intrinsics.js
+++ b/lib/internal/freeze_intrinsics.js
@@ -65,6 +65,7 @@ const {
   SymbolIterator,
   SyntaxError,
   TypeError,
+  TypedArrayPrototype,
   Uint16Array,
   Uint32Array,
   Uint8Array,
@@ -105,7 +106,7 @@ module.exports = function() {
     // AsyncGeneratorFunction
     ObjectGetPrototypeOf(async function* () {}),
     // TypedArray
-    ObjectGetPrototypeOf(Uint8Array),
+    TypedArrayPrototype,
 
     // 19 Fundamental Objects
     Object.prototype, // 19.1
@@ -189,7 +190,7 @@ module.exports = function() {
     // AsyncGeneratorFunction
     ObjectGetPrototypeOf(async function* () {}),
     // TypedArray
-    ObjectGetPrototypeOf(Uint8Array),
+    TypedArrayPrototype,
 
     // 18 The Global Object
     eval,

ExE-Boss · 2020-11-08T01:07:42Z

lib/internal/per_context/primordials.js

+  const original = global[name];
+  primordials[name] = original;
+  copyPropsRenamed(original, primordials, name);


This is needed because of #35448 (comment).

Can you open a separate PR for this?

freeze_intrinsics.js is executed before any user code, I don't think it's worth adding it to the primordials object: there's no security reason to do so, IMHO we should fix frozen_intrinsics.js code instead.

lib/internal/freeze_intrinsics.js

aduh95 · 2020-11-08T09:06:35Z

lib/internal/per_context/primordials.js

+  const original = global[name];
+  primordials[name] = original;
+  copyPropsRenamed(original, primordials, name);


Can you open a separate PR for this?

freeze_intrinsics.js is executed before any user code, I don't think it's worth adding it to the primordials object: there's no security reason to do so, IMHO we should fix frozen_intrinsics.js code instead.

Refs: nodejs#35448 Refs: nodejs#36003 Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot · 2020-11-08T19:34:02Z

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

github-actions · 2020-11-09T09:41:18Z

Landed in aa1eb1f...c925e24

Refs: #35448 Refs: #36003 Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #36016 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com>

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Nov 7, 2020

ExE-Boss mentioned this pull request Nov 7, 2020

child_process: refactor to use more primordials #36003

Closed

2 tasks

aduh95 approved these changes Nov 7, 2020

View reviewed changes

lib/internal/per_context/primordials.js Show resolved Hide resolved

lib/internal/per_context/primordials.js Show resolved Hide resolved

benjamingr approved these changes Nov 7, 2020

View reviewed changes

ExE-Boss force-pushed the lib/primordials/add-typed-array branch from 06d4cde to 4c11653 Compare November 7, 2020 09:19

ExE-Boss requested a review from aduh95 November 7, 2020 09:21

aduh95 reviewed Nov 7, 2020

View reviewed changes

lib/internal/per_context/primordials.js Show resolved Hide resolved

ExE-Boss requested a review from aduh95 November 7, 2020 09:34

aduh95 approved these changes Nov 7, 2020

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 7, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2020

This comment has been minimized.

# to view

ExE-Boss force-pushed the lib/primordials/add-typed-array branch from 4c11653 to 15592ff Compare November 8, 2020 01:04

ExE-Boss mentioned this pull request Nov 8, 2020

Add new ESLint rule: prefer-primordials #35448

Closed

3 tasks

ExE-Boss commented Nov 8, 2020

View reviewed changes

ExE-Boss force-pushed the lib/primordials/add-typed-array branch from 15592ff to 6d64af7 Compare November 8, 2020 01:09

ExE-Boss requested a review from aduh95 November 8, 2020 01:10

aduh95 reviewed Nov 8, 2020

View reviewed changes

Leko approved these changes Nov 8, 2020

View reviewed changes

aduh95 mentioned this pull request Nov 8, 2020

lib: add TypedArray to the primordials #32127

Closed

2 tasks

ExE-Boss force-pushed the lib/primordials/add-typed-array branch 2 times, most recently from aca2ee7 to 8fee9b7 Compare November 8, 2020 19:27

ExE-Boss requested a review from aduh95 November 8, 2020 19:27

lib: add %TypedArray% abstract constructor to primordials

8fee9b7

Refs: nodejs#35448 Refs: nodejs#36003 Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

aduh95 approved these changes Nov 8, 2020

View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020

github-actions bot closed this Nov 9, 2020

ExE-Boss deleted the lib/primordials/add-typed-array branch November 9, 2020 10:00

danielleadams mentioned this pull request Nov 9, 2020

v15.2.0 proposal #36055

Merged

targos mentioned this pull request Jun 6, 2021

v14.17.1 release proposal #38948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: add %TypedArray% abstract constructor to primordials #36016

lib: add %TypedArray% abstract constructor to primordials #36016

ExE-Boss commented Nov 7, 2020 •

edited

Loading

aduh95 left a comment

benjamingr left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aduh95 commented Nov 7, 2020

ExE-Boss Nov 8, 2020

aduh95 Nov 8, 2020

aduh95 Nov 8, 2020

nodejs-github-bot commented Nov 8, 2020

github-actions bot commented Nov 9, 2020

lib: add %TypedArray% abstract constructor to primordials #36016

lib: add %TypedArray% abstract constructor to primordials #36016

Conversation

ExE-Boss commented Nov 7, 2020 • edited Loading

Checklist

aduh95 left a comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aduh95 commented Nov 7, 2020

ExE-Boss Nov 8, 2020

Choose a reason for hiding this comment

aduh95 Nov 8, 2020

Choose a reason for hiding this comment

aduh95 Nov 8, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 8, 2020

github-actions bot commented Nov 9, 2020

ExE-Boss commented Nov 7, 2020 •

edited

Loading