util: freeze `kEnumerableProperty` #43390

LiviaMedeiros · 2022-06-12T06:09:38Z

nodejs-github-bot · 2022-06-12T06:49:34Z

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

aduh95 · 2022-06-12T08:15:34Z

test/parallel/test-internal-util-objects.js

+  assert.strictEqual(
+    Object.isExtensible(kEnumerableProperty),
+    false
+  );
+  assert.strictEqual(
+    Object.isSealed(kEnumerableProperty),
+    true
+  );
+  assert.strictEqual(
+    Object.isFrozen(kEnumerableProperty),
+    true
+  );


This doesn't really seem like something we need to care in our tests (in no credible scenario will anyone tries to check what's the value of Object.isSealed(kEnumerableProperty) in core, and if there was a way to make sure the object always behaves as we want it to that doesn't involve freezing it, that'd be fine too), the consequences are what's really important to test imo:

Suggested change

assert.strictEqual(

Object.isExtensible(kEnumerableProperty),

false

);

assert.strictEqual(

Object.isSealed(kEnumerableProperty),

true

);

assert.strictEqual(

Object.isFrozen(kEnumerableProperty),

true

);

assert.throws(

() => { kEnumerableProperty.configurable = false; },

TypeError

);

assert.throws(

() => Object.assign(kEnumerableProperty, { configurable: false }),

TypeError

);

assert.throws(

() => Object.assign(kEnumerableProperty, { enumerable: false }),

TypeError

);

Conceptually agreed. :)
My concern here is that by assuming alternatives to freezing and possibility of something dangerous (e.g. if there will be unavoidable exposing reference to kEnumerableProperty for userland) we must keep in mind every single way to break it.
If that approach will become common in core, we'll have to create an exhaustive isImmutable() helper function instead.

nodejs-github-bot · 2022-06-12T09:25:42Z

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

LiviaMedeiros · 2022-06-13T10:56:22Z

Looks like CI got borked recently

nodejs-github-bot · 2022-06-13T13:38:18Z

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

nodejs-github-bot · 2022-06-15T04:49:57Z

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

PR-URL: nodejs#43390 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Beth Griggs <bgriggs@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

LiviaMedeiros · 2022-06-15T08:42:34Z

Landed in 0f90879

PR-URL: #43390 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Beth Griggs <bgriggs@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: nodejs/node#43390 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Beth Griggs <bgriggs@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jun 12, 2022

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2022

aduh95 reviewed Jun 12, 2022

View reviewed changes

aduh95 approved these changes Jun 12, 2022

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 Jun 12, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2022

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2022

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2022

This comment was marked as duplicate.

Sign in to view

benjamingr approved these changes Jun 13, 2022

View reviewed changes

jasnell approved these changes Jun 13, 2022

View reviewed changes

github-actions bot mentioned this pull request Jun 14, 2022

CI Reliability 2022-06-14 nodejs/reliability#300

Open

16 tasks

LiviaMedeiros force-pushed the util-freeze-kenobirable-property branch from 174856c to 7a84b9b Compare June 14, 2022 15:07

This comment was marked as outdated.

Sign in to view

BethGriggs approved these changes Jun 14, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

lpinca approved these changes Jun 14, 2022

View reviewed changes

github-actions bot mentioned this pull request Jun 15, 2022

CI Reliability 2022-06-15 nodejs/reliability#301

Open

17 tasks

LiviaMedeiros force-pushed the util-freeze-kenobirable-property branch from 7a84b9b to 0f90879 Compare June 15, 2022 08:42

LiviaMedeiros merged commit 0f90879 into nodejs:master Jun 15, 2022

github-actions bot mentioned this pull request Jun 16, 2022

CI Reliability 2022-06-16 nodejs/reliability#302

Open

22 tasks

danielleadams mentioned this pull request Jun 16, 2022

v18.4.0 proposal #43385

Merged

github-actions bot mentioned this pull request Jun 17, 2022

CI Reliability 2022-06-17 nodejs/reliability#303

Open

31 tasks

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: freeze `kEnumerableProperty` #43390

util: freeze `kEnumerableProperty` #43390

LiviaMedeiros commented Jun 12, 2022

nodejs-github-bot commented Jun 12, 2022

aduh95 Jun 12, 2022 •

edited

Loading

LiviaMedeiros Jun 12, 2022

nodejs-github-bot commented Jun 12, 2022

LiviaMedeiros commented Jun 13, 2022

nodejs-github-bot commented Jun 13, 2022 •

edited by LiviaMedeiros

Loading

This comment was marked as duplicate.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Jun 15, 2022

LiviaMedeiros commented Jun 15, 2022

-  assert.strictEqual(
-    Object.isExtensible(kEnumerableProperty),
-    false
-  );
-  assert.strictEqual(
-    Object.isSealed(kEnumerableProperty),
-    true
-  );
-  assert.strictEqual(
-    Object.isFrozen(kEnumerableProperty),
-    true
-  );
+  assert.throws(
+    () => { kEnumerableProperty.configurable = false; },
+    TypeError
+  );
+  assert.throws(
+    () => Object.assign(kEnumerableProperty, { configurable: false }),
+    TypeError
+  );
+  assert.throws(
+    () => Object.assign(kEnumerableProperty, { enumerable: false }),
+    TypeError
+  );

util: freeze kEnumerableProperty #43390

util: freeze kEnumerableProperty #43390

Conversation

LiviaMedeiros commented Jun 12, 2022

nodejs-github-bot commented Jun 12, 2022

aduh95 Jun 12, 2022 • edited Loading

Choose a reason for hiding this comment

LiviaMedeiros Jun 12, 2022

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 12, 2022

LiviaMedeiros commented Jun 13, 2022

nodejs-github-bot commented Jun 13, 2022 • edited by LiviaMedeiros Loading

This comment was marked as duplicate.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Jun 15, 2022

LiviaMedeiros commented Jun 15, 2022

util: freeze `kEnumerableProperty` #43390

util: freeze `kEnumerableProperty` #43390

aduh95 Jun 12, 2022 •

edited

Loading

nodejs-github-bot commented Jun 13, 2022 •

edited by LiviaMedeiros

Loading