Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent enumerability #20565

Closed
BridgeAR opened this issue May 6, 2018 · 10 comments
Closed

Inconsistent enumerability #20565

BridgeAR opened this issue May 6, 2018 · 10 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@BridgeAR
Copy link
Member

BridgeAR commented May 6, 2018

When looking at the global object we have quite a few properties that are not enumerable and we have a couple that are.

Using Object.keys(global) currently results in:

[ 'global',
  'process',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout',
  'module',
  'require' ]

URL and console are for example not enumerable. Should we maybe reconsider these and either set everything to being enumerable / not enumerable? Or should we just define that everything added from now will be not enumerable?

I could also not find any issue that was directly about this before. Somewhat related: #8810

@BridgeAR BridgeAR added the meta Issues and PRs related to the general management of the project. label May 6, 2018
@devsnek
Copy link
Member

devsnek commented May 7, 2018

timers are enumerable by spec and most if not all of the whatwg globals are unenumerable by spec

@BridgeAR
Copy link
Member Author

BridgeAR commented May 7, 2018

I know about timers and the whatwg ones. But that still leaves:

global
process
Buffer
module
require

@TimothyGu
Copy link
Member

See #10405 for global. module and require are only globals in the REPL. The other two can go either way, but I don’t see any strong argument to change the status quo.

I think we could close this issue.

@benjamingr
Copy link
Member

benjamingr commented May 7, 2018

timers are enumerable by spec

what spec?

AFAIK we don't have a spec.

@benjamingr
Copy link
Member

Honestly I think there isn't a lot of value and there is a lot of breakage potential in making things enumerable/not-enumerable.

I think we should have a "new things are not enumerable" policy overall - and that anyone doing Object.keys(process) are doing something extreme enough so that Object.getOwnPropertyNames won't be a big deal.

@bnoordhuis
Copy link
Member

what spec?

https://html.spec.whatwg.org/ - timers (and over 200 other window properties) are enumerable.

@ChALkeR
Copy link
Member

ChALkeR commented May 7, 2018

Chromium 66.0.3359.139 output for comparison:

> Object.keys(window).filter(x => !/^(on|webkit|screen|page|scroll)/.test(x)).sort()
(75) [ "alert", "applicationCache", "atob", "blur", "btoa", "cancelAnimationFrame",
"cancelIdleCallback", "captureEvents", "chrome", "clearInterval", "clearTimeout",
"clientInformation", "close", "closed", "confirm", "createImageBitmap", "crypto",
"customElements", "defaultStatus", "defaultstatus", "devicePixelRatio", "document",
"external", "fetch", "find", "focus", "frameElement", "frames", "getComputedStyle",
"getSelection", "history", "indexedDB", "innerHeight", "innerWidth", "isSecureContext",
"length", "localStorage", "location", "locationbar", "matchMedia", "menubar", "moveBy",
"moveTo", "name", "navigator", "open", "openDatabase", "opener", "origin", "outerHeight",
"outerWidth", "parent", "performance", "personalbar", "postMessage", "print", "prompt",
"releaseEvents", "requestAnimationFrame", "requestIdleCallback", "resizeBy", "resizeTo",
"self", "sessionStorage", "setInterval", "setTimeout", "speechSynthesis", "status",
"statusbar", "stop", "styleMedia", "toolbar", "top", "visualViewport", "window" ]

@benjamingr
Copy link
Member

benjamingr commented May 7, 2018

@bnoordhuis

https://html.spec.whatwg.org/ - timers (and over 200 other window properties) are enumerable.

We don't follow that spec at all though, not with regards to limits, edge cases, return values and so on. If we want to consider following the DOM timer spec then we should probably work towards those goals.

I'm a maintainer in a fake timer library and there is quite a bit of code that deals with differences between Node and browsers :)

@BridgeAR BridgeAR mentioned this issue May 14, 2018
4 tasks
@BridgeAR
Copy link
Member Author

global. module and require are not enumerable anymore in the REPL. That reduces the list of globals that are questionable to: process and Buffer.

@Trott
Copy link
Member

Trott commented Nov 6, 2018

It seems like perhaps this should be closed, as the remaining items are debatable? Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree (or open a PR). I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Nov 6, 2018
BridgeAR added a commit to BridgeAR/node that referenced this issue Dec 6, 2018
This makes sure these two properties are non-enumerable. This aligns
them with all other globals that are not enumerable by spec.

Refs: nodejs#20565
BridgeAR added a commit to BridgeAR/node that referenced this issue Dec 13, 2018
This makes sure these two properties are non-enumerable. This aligns
them with all other globals that are not enumerable by spec.

Refs: nodejs#20565

PR-URL: nodejs#24874
Refs: nodejs#20565
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
This makes sure these two properties are non-enumerable. This aligns
them with all other globals that are not enumerable by spec.

Refs: nodejs#20565

PR-URL: nodejs#24874
Refs: nodejs#20565
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

7 participants