doc: replace functions with arrow functions #6203
Conversation
| @@ -500,7 +500,7 @@ If you need to support older versions and don't need the worker object, | |||
| you can work around the discrepancy by checking the number of arguments: | |||
|
|
|||
| ```js | |||
| cluster.on('message', function(worker, message, handle) { | |||
| cluster.on('message', (worker, message, handle) => { | |||
| if (arguments.length === 2) { | |||
There was a problem hiding this comment.
arrow functions do not have the arguments so this wouldn't work.
There was a problem hiding this comment.
sorry, I overlooked arguments.
hiroppy
force-pushed
the
replace-functions-with-arrow-functions
branch
from
404cc96
to
1792431
Compare
| @@ -240,7 +240,7 @@ formatted according to the returned Object. This is similar to how | |||
|
|
|||
| ```js | |||
| var obj = { foo: 'this will not show up in the inspect() output' }; | |||
| obj.inspect = function(depth) { | |||
| obj.inspect = (depth) => { | |||
There was a problem hiding this comment.
I don't think we should replace them in this file, personally.
hiroppy
force-pushed
the
replace-functions-with-arrow-functions
branch
from
1792431
to
f5ae1a6
Compare
|
I am not sure if we really need this to be changed. |
|
There's likely not a pressing need but it also doesn't hurt. LGTM |
|
I recall some discussion in @nodejs/documentation about some ES6 features, like arrow functions, potentially making examples unclear to those that aren't yet fully familiar with the features. Did we ever come to a consensus on arrow functions? I know the different semantics of |
|
Perhaps we need to take some time in the examples to explain the differences then? Perhaps show the example in both forms? |
|
I did a PR a while ago for having a visual ES5/ES6 toggle. We rejected it for the amount of duplicated code in the docs back then. We wanted to revisit it at some later time. If this would be interesting again, you guys could put it on the agenda. For now we favor arrow functions though. Ref: #4915 |
|
Hmm.. perhaps a doc/topics addition then? Something that explains the various nuances that exist between ES5 and ES6 ways of doing things. |
|
@eljefedelrodeodeljefe the discussion in the WG meeting was about backticks being visually confusing. I don't think anyone ever suggested discussing it in a docs WG meeting before (but I might be wrong). That said, these sort of PRs are usually rejected for "creating churn" and not being worth the process around them. Personally I think they're fine but from what I recall that was the consensus. |
|
While they certainly do create a bit of churn, this kind of small doc focused PR is perfect for people who are just getting started in making contributions to Node. They may not be significant changes in their own right, but could open the door to more impactful changes later on. |
|
@jasnell to clarify, I personally agree with that, I was just under the impression my opinion was an unpopular one in Node. I think that given how much we spend trying to encourage contribution getting people accustomed to the PR process is a good thing in its own right. |
|
Okay. Let's put this on the agenda of the docs WG then. @jasnell are you rather in favor of having small but plenty or big topic/guide? |
|
I would start small-ish and iterate from there. |
+1 on this; pull requests are intended to invoke discussion. As log as there's plenty of people who are willing to lend a hand to newcomers and we have high conformance with style guides, it should be manageable. |
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 9a9beef |
|
Just wanted to add a late 👍 to all that has been discussed. My comment before was not meant to be in protest of this PR or anything, just wanted to be sure we had some defined opinion on arrow functions. Also agreed that style changes in docs probably shouldn't be considered churn, since it's presentational content rather than operational content. |
|
Thanks for the follow-up. Starting a discussion about this is the good and right thing 👍 |
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#6203 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6203 Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
Affected core subsystem(s)
doc
Description of change
I replaced "functions" syntax with "arrow functions" syntax.