domain: Make `.domain` on async resources non-enumerable by ljharb · Pull Request #26210 · nodejs/node · GitHub

In particular, this comes into play in the node repl, which apparently enables domains by default. Whenever any Promise gets inspected, a .domain property is displayed, which is very confusing, especially since it has some kind of WeakReference attached to it, which is not yet a language feature.

This change will prevent it from showing up in casual inspection, but will leave it available for use.

I have not run the tests locally yet, because they're incredibly slow and I need to do other things with my machine, and because the last time I tried some of them failed on master anyways.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

cc @addaleax @devsnek

In particular, this comes into play in the node repl, which apparently enables domains by default. Whenever any Promise gets inspected, a `.domain` property is displayed, which is *very confusing*, especially since it has some kind of WeakReference attached to it, which is not yet a language feature. This change will prevent it from showing up in casual inspection, but will leave it available for use.

LGTM

/cc @BridgeAR

LGTM. It would be nice to add a test to it as well.

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

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20973/

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

LGTM!

Does this need tests?

What's the semver status of this? I don't think this is a major, but is it a minor or patch?

I’d call it a patch; imo it fixes confusion in the repl.

@ljharb would you mind adding a test?

I have not run the tests locally yet, because they're incredibly slow and I need to do other things with my machine, and because the last time I tried some of them failed on master anyways.

@ljharb If you want to only run domain tests, you can do tools/test.py domain. On my machine, that runs 50 test files in 11 seconds. (No failures on master branch.)

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/21223/

@BridgeAR I've added some; let me know if that's not what you had in mind :-)

@Trott

no dice :-/

$ tools/test.py domain
Traceback (most recent call last):
  File "tools/test.py", line 1766, in <module>
    sys.exit(Main())
  File "tools/test.py", line 1639, in Main
    vm = context.GetVm(arch, mode)
  File "tools/test.py", line 953, in GetVm
    raise ValueError('Could not find executable. Should be ' + name)
ValueError: Could not find executable. Should be out/Release/node

@ljharb you have to compile Node.js first. Otherwise the tests can't run.

@ljharb I just started a CI and it seems like there are linter errors.

@BridgeAR thanks, fixed.

Do we really need all of these defineProperty calls? If I am not mistaken we only have to set the property to enumerable false once and every following assignment will keep the descriptor in place.

@BridgeAR certainly you're right, but for all of the objects that aren't previously defined - like newly created resources, errors, event emitters, etc - it'd need the defineProperty every time.

@BridgeAR updated

CI https://ci.nodejs.org/job/node-test-pull-request/21481/ (:white_check_mark:)

@misterdjules do you have any thoughts on this?

As far as semver - this could be considered semver-major if someone was using the domains flag, and also enumerating an async resource, and then relying on domain to be present there - but I can't imagine why anyone would be doing that. That would make this either a major or a patch, depending on the level of conservatism.

Landed in 377c583 :)

In particular, this comes into play in the node repl, which apparently enables domains by default. Whenever any Promise gets inspected, a `.domain` property is displayed, which is *very confusing*, especially since it has some kind of WeakReference attached to it, which is not yet a language feature. This change will prevent it from showing up in casual inspection, but will leave it available for use. PR-URL: #26210 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

In particular, this comes into play in the node repl, which apparently enables domains by default. Whenever any Promise gets inspected, a `.domain` property is displayed, which is *very confusing*, especially since it has some kind of WeakReference attached to it, which is not yet a language feature. This change will prevent it from showing up in casual inspection, but will leave it available for use. PR-URL: nodejs#26210 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

In particular, this comes into play in the node repl, which apparently enables domains by default. Whenever any Promise gets inspected, a `.domain` property is displayed, which is *very confusing*, especially since it has some kind of WeakReference attached to it, which is not yet a language feature. This change will prevent it from showing up in casual inspection, but will leave it available for use. PR-URL: #26210 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@MylesBorins This is great!

I ran into this recently when logging restify errors created from within a domain and we had to filter the domain property out. Having that property not enumerable would have avoided that issue in the first place.

And thanks for pinging me here, very much appreciated ❤️

nodejs-github-bot added the domain Issues and PRs related to the domain subsystem. label Feb 20, 2019

addaleax approved these changes Feb 20, 2019

View reviewed changes

ljharb force-pushed the domain_non_enumerable branch 2 times, most recently from 9d0013e to ed34d24 Compare February 20, 2019 00:46

BridgeAR approved these changes Feb 20, 2019

View reviewed changes

targos approved these changes Feb 20, 2019

View reviewed changes

cjihrig approved these changes Feb 20, 2019

View reviewed changes

devsnek approved these changes Feb 20, 2019

View reviewed changes

lpinca approved these changes Feb 20, 2019

View reviewed changes

jasnell approved these changes Feb 21, 2019

View reviewed changes

ljharb force-pushed the domain_non_enumerable branch 2 times, most recently from b233a06 to 084f7b2 Compare February 22, 2019 07:40

ljharb force-pushed the domain_non_enumerable branch 2 times, most recently from 11c1de7 to caa10ed Compare February 27, 2019 04:02

ChALkeR approved these changes Mar 1, 2019

View reviewed changes

ljharb force-pushed the domain_non_enumerable branch from caa10ed to 311cec0 Compare March 3, 2019 07:08

ljharb force-pushed the domain_non_enumerable branch from 311cec0 to 9be90d9 Compare March 5, 2019 22:13

This comment has been minimized.

Sign in to view

ljharb force-pushed the domain_non_enumerable branch from 9be90d9 to 5aca8a1 Compare March 6, 2019 06:53

ljharb force-pushed the domain_non_enumerable branch from 5aca8a1 to 3308853 Compare March 8, 2019 23:56

BridgeAR reviewed Mar 10, 2019

View reviewed changes

Comment thread lib/domain.js Outdated

Comment thread lib/domain.js Outdated

ljharb force-pushed the domain_non_enumerable branch from 3308853 to b1ae6f4 Compare March 11, 2019 06:19

BridgeAR approved these changes Mar 11, 2019

View reviewed changes

devsnek added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 12, 2019

tniessen approved these changes Mar 12, 2019

View reviewed changes

addaleax closed this Mar 13, 2019

ljharb deleted the domain_non_enumerable branch March 13, 2019 06:00

BridgeAR mentioned this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current) #26644

Merged

domain: Make `.domain` on async resources non-enumerable by ljharb · Pull Request #26210 · nodejs/node · GitHub

Uh oh!

Uh oh!

domain: Make .domain on async resources non-enumerable - #26210

domain: Make .domain on async resources non-enumerable#26210ljharb wants to merge 1 commit intonodejs:masternodejs/node:masterfrom ljharb:domain_non_enumerableljharb/node:domain_non_enumerableCopy head branch name to clipboard

Conversation

ljharb commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Feb 22, 2019

Uh oh!

Trott commented Feb 22, 2019

Uh oh!

addaleax commented Mar 1, 2019

Uh oh!

ChALkeR left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb commented Mar 1, 2019

Uh oh!

BridgeAR commented Mar 5, 2019

Uh oh!

Trott commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Mar 5, 2019

Uh oh!

ljharb commented Mar 5, 2019

Uh oh!

This comment has been minimized.

BridgeAR commented Mar 5, 2019

Uh oh!

BridgeAR commented Mar 5, 2019

Uh oh!

ljharb commented Mar 6, 2019

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

ljharb commented Mar 10, 2019

Uh oh!

Uh oh!

Uh oh!

ljharb commented Mar 11, 2019

Uh oh!

devsnek commented Mar 12, 2019 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Mar 12, 2019

Uh oh!

ljharb commented Mar 12, 2019

Uh oh!

addaleax commented Mar 13, 2019

Uh oh!

misterdjules commented Mar 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

domain: Make .domain on async resources non-enumerable#26210
ljharb wants to merge 1 commit into
nodejs:masterfrom
ljharb:domain_non_enumerable

ljharb commented Feb 20, 2019 •
edited
Loading

ChALkeR left a comment •
edited
Loading

Trott commented Mar 5, 2019 •
edited
Loading

devsnek commented Mar 12, 2019 •
edited by addaleax
Loading