doc: added symbols guidelines #22684

mcollina · 2018-09-04T10:36:04Z

I've added the Symbol guideline that spun out from the discussion in #20857.

Checklist

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

nodejs-github-bot · 2018-09-04T10:36:05Z

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/789/pipeline

mcollina · 2018-09-04T10:37:05Z

cc @devsnek @nodjes/tsc.

This also is relevant for #22302 (feature detection).

jasnell · 2018-09-04T16:24:14Z

doc/guides/using-symbols.md

nit: s/makes/make

richardlau · 2018-09-04T18:00:10Z

cc @devsnek @nodjes/tsc.

This also is relevant for #22302 (feature detection).

@nodejs/tsc 😄

mcollina · 2018-09-04T20:42:27Z

Tagging it tsc-agenda to raise awareness. cc @nodejs/tsc.

cjihrig

Some of the content could probably be replaced with a link to MDN, but LGTM. A link to MDN probably wouldn't hurt either way though 😄

BridgeAR · 2018-09-04T20:51:48Z

doc/guides/using-symbols.md

s/thay/they/

BridgeAR · 2018-09-04T20:54:35Z

doc/guides/using-symbols.md

s/Globals/Global/

I suggest to rephrase this sentence to something like: Global symbols are used in Node.js to allow customization of certain behaviors, i.e., metaprogramming.

doc/guides/using-symbols.md

vsemozhetbyt · 2018-09-04T21:51:51Z

doc/guides/using-symbols.md

as they is a bit confusing since there are no plurals before (except purposes).

vsemozhetbyt · 2018-09-04T21:53:18Z

doc/guides/using-symbols.md

globals -> global?

doc/guides/using-symbols.md

vsemozhetbyt · 2018-09-04T22:07:54Z

doc/guides/using-symbols.md

Symbol.for -> Symbol.for() ?

TimothyGu · 2018-09-05T00:49:54Z

doc/guides/using-symbols.md

Symbols in the GlobalSymbolRegistry have no intrinsic differences from symbols created through Symbol() -- other than that it is in a global map. There are no two "types" of symbols.

From a user point of view, those are two different things. It might well be the same type, but a) I cannot add a symbol that I created to the global map and b) I cannot remove a symbol from the global map. Specifically Symbol('hello') !== Symbol('hello') and Symbol.for('hello') === Symbol.for('hello'): this alone makes me think that they are two different things. Would you mind to suggest some different wording?

doc/guides/using-symbols.md

TimothyGu · 2018-09-05T00:51:25Z

doc/guides/using-symbols.md

There is no such thing as a "local" Symbol. Just say symbols created using the Symbol() function.

TimothyGu · 2018-09-05T00:52:39Z

doc/guides/using-symbols.md

Say why this is good. If I were an unsuspecting developer I might consider

make it harder for developers to monkey patch/access private fields

a bad thing and question why Node.js is doing this.

TimothyGu · 2018-09-05T00:55:22Z

doc/guides/using-symbols.md

Drive home the fact that the symbol is intended developer-facing.

Global symbols should be preferred when a developer-facing interface is needed to allow behavior customization, i.e., metaprogramming.

TimothyGu · 2018-09-05T00:56:34Z

doc/guides/using-symbols.md

I'd also clarify what "global" mean here: they are global to all contexts (including VM contexts) in the same process/JavaScript agent/V8 Isolate.

i would just say v8 isolate

addaleax · 2018-09-05T09:34:41Z

doc/guides/using-symbols.md

Please make this Symbol('kField'). Having consistent naming is so helpful for grep-ping, and I’ve been changing it to this pattern whenever I touched these lines.

mcollina · 2018-09-05T12:23:40Z

PTAL @TimothyGu @addaleax.

BridgeAR · 2018-09-05T13:44:14Z

doc/guides/using-symbols.md

s/imoprove/improve/

BridgeAR · 2018-09-05T13:45:43Z

doc/guides/using-symbols.md

-breaking +causing issues for

danbev · 2018-09-07T07:28:54Z

Landed in 4f5aa36.

PR-URL: #22684 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Refs: nodejs#22684

PR-URL: #22684 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

tniessen · 2018-09-07T20:37:56Z

@BridgeAR suggested to make symbols non-enumerable for private fields in #22747 (comment). If we decide to do that, we should probably add to these guidelines, so I would love to hear opinions.

mcollina · 2018-09-07T22:18:35Z

I’m not super fond of that, because we would have to use defineProperty instead of a basic assignment in class definitions and in a lot of property assignment. It’d make the code less maintainable over time and it might cause some performance loss.

jasnell · 2018-09-07T22:20:33Z

I'm not particularly fond of it either. While I was not really happy with the change that caused symbols to be included in the inspect output, if we really want to hide those details we can do so by providing a custom inspect implementation.

BridgeAR · 2018-09-07T22:31:40Z

I don't think that the code becomes less maintainable by using Object.defineProperty but it would likely be a tiny performance overhead in case it's a hot function. The assignment itself is normally in the constructor and that should be fine.

A custom inspect function is not ideal since we would have to reimplement a couple of features every time and that's significantly more work and code than just switching to non-enumerable properties.

mcollina · 2018-09-08T10:25:57Z

The refactoring needed to https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js would be massive, and definitely hinder its readability.

As an example

node/lib/internal/http2/core.js

Lines 1566 to 1595 in 922a1b0

    
           [kInit](id, handle) { 
        
             const state = this[kState]; 
        
             state.flags |= STREAM_FLAGS_READY; 
        
             const session = this[kSession]; 
        
             session[kState].pendingStreams.delete(this); 
        
             session[kState].streams.set(id, this); 
        
             this[kID] = id; 
        
             this[async_id_symbol] = handle.getAsyncId(); 
        
             handle[kOwner] = this; 
        
             this[kHandle] = handle; 
        
             handle.ontrailers = onStreamTrailers; 
        
             handle.onstreamclose = onStreamClose; 
        
             handle.onread = onStreamRead; 
        
             this.uncork(); 
        
             this.emit('ready'); 
        
           } 
        
           [kInspect](depth, opts) { 
        
             const obj = { 
        
               id: this[kID] || '<pending>', 
        
               closed: this.closed, 
        
               destroyed: this.destroyed, 
        
               state: this.state, 
        
               readableState: this._readableState, 
        
               writableState: this._writableState 
        
             }; 
        
             return `Http2Stream ${util.format(obj)}`; 
        
           }

would need to be moved out of the class definition, because we cannot declare non-enumerable fields there.

PR-URL: nodejs#22770 Refs: nodejs#22684 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #22770 Refs: #22684 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Define `util.inspect.custom` as `Symbol.for("nodejs.util.inspect.custom")` rather than `Symbol("util.inspect.custom")`. This allows `inspect` hooks to easily/safely be defined in non-Node.js environments. Fixes: #20821 Refs: #22684 PR-URL: #20857 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: John-David Dalton <[email protected]>

PR-URL: #22770 Refs: #22684 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Define `util.inspect.custom` as `Symbol.for("nodejs.util.inspect.custom")` rather than `Symbol("util.inspect.custom")`. This allows `inspect` hooks to easily/safely be defined in non-Node.js environments. Fixes: nodejs#20821 Refs: nodejs#22684 PR-URL: nodejs#20857 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: John-David Dalton <[email protected]>

Define `util.inspect.custom` as `Symbol.for("nodejs.util.inspect.custom")` rather than `Symbol("util.inspect.custom")`. This allows `inspect` hooks to easily/safely be defined in non-Node.js environments. Fixes: #20821 Refs: #22684 Backport-PR-URL: #23039 PR-URL: #20857 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: John-David Dalton <[email protected]>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 4, 2018

tniessen approved these changes Sep 4, 2018

View reviewed changes

jasnell reviewed Sep 4, 2018

View reviewed changes

doc/guides/using-symbols.md Outdated

Copy link

Member

jasnell Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/makes/make

jasnell approved these changes Sep 4, 2018

View reviewed changes

mcollina force-pushed the symbols-guideline branch from 801d47f to 40ec16b Compare September 4, 2018 20:42

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 4, 2018

mcollina requested a review from a team September 4, 2018 20:42

mcollina added the meta Issues and PRs related to the general management of the project. label Sep 4, 2018

cjihrig approved these changes Sep 4, 2018

View reviewed changes

BridgeAR approved these changes Sep 4, 2018

View reviewed changes

lpinca reviewed Sep 4, 2018

View reviewed changes

doc/guides/using-symbols.md Outdated Show resolved Hide resolved

lpinca approved these changes Sep 4, 2018

View reviewed changes

vsemozhetbyt approved these changes Sep 4, 2018

View reviewed changes

TimothyGu reviewed Sep 5, 2018

View reviewed changes

danbev approved these changes Sep 5, 2018

View reviewed changes

addaleax reviewed Sep 5, 2018

View reviewed changes

mcollina force-pushed the symbols-guideline branch from 40ec16b to a9d210a Compare September 5, 2018 12:23

BridgeAR reviewed Sep 5, 2018

View reviewed changes

doc: added symbols guidelines

0d9162a

mcollina force-pushed the symbols-guideline branch from a9d210a to 0d9162a Compare September 5, 2018 13:54

addaleax approved these changes Sep 5, 2018

View reviewed changes

mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 5, 2018

apapirovski approved these changes Sep 5, 2018

View reviewed changes

danbev closed this Sep 7, 2018

mcollina deleted the symbols-guideline branch September 7, 2018 07:59

mcollina mentioned this pull request Sep 7, 2018

util: use a shared symbol for util.inspect.custom #20857

Closed

4 tasks

tniessen added a commit to tniessen/node that referenced this pull request Sep 7, 2018

crypto: rename symbols to match guidelines

251696e

Refs: nodejs#22684

tniessen mentioned this pull request Sep 7, 2018

crypto: hide native handles from JS modules #22747

Closed

2 tasks

tniessen mentioned this pull request Sep 9, 2018

crypto: rename symbols to match guidelines #22770

Closed

2 tasks

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

vweevers mentioned this pull request Jan 5, 2023

stream: expose stream symbols #45671

Merged

Uh oh!

doc: added symbols guidelines #22684

doc: added symbols guidelines #22684

Uh oh!

Conversation

mcollina commented Sep 4, 2018

Checklist

Uh oh!

nodejs-github-bot commented Sep 4, 2018

Uh oh!

mcollina commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardlau commented Sep 4, 2018

Uh oh!

mcollina commented Sep 4, 2018

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Sep 7, 2018

Uh oh!

tniessen commented Sep 7, 2018

Uh oh!

mcollina commented Sep 7, 2018

Uh oh!

jasnell commented Sep 7, 2018

Uh oh!

BridgeAR commented Sep 7, 2018

Uh oh!

mcollina commented Sep 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants