Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Code And Learn at NodeFest 2017 #72

Closed
hiroppy opened this issue Oct 18, 2017 · 32 comments
Closed

Code And Learn at NodeFest 2017 #72

hiroppy opened this issue Oct 18, 2017 · 32 comments

Comments

@hiroppy
Copy link
Member

hiroppy commented Oct 18, 2017

Hi!

We will hold NodeFest which is the largest Node.js conference in Japan.
I’m one of the organizers this year.
The team members are @yosuke-furukawa, @watilde, myself and some other great nodeschool staff.

We would like to open code-and-learn-jp on NodeFest and support the contribution to nodejs/core.

Please feel free to suggest any contribution areas for the participants! @nodejs/collaborators
We will receive some PRs on 26th November.(http://nodefest.jp/2017/schedule.html)

/cc @yosuke-furukawa @watilde @nodejs/nodejs-ja

Thanks.

Mentors

@joyeecheung
Copy link
Member

cc @Trott @addaleax

Also, I will be there. Happy to help out with whatever needed.

@ronkorving
Copy link

Of course I will be there 👍

@vsemozhetbyt
Copy link

vsemozhetbyt commented Oct 18, 2017

Possible case in tests: nodejs/node#16243 (comment) + next comment: check else if by if replacements when a previous if contains return. Also check else eliminations on similar condition.

@hiroppy
Copy link
Member Author

hiroppy commented Oct 18, 2017

@vsemozhetbyt Thank you for your information!

@watilde
Copy link
Member

watilde commented Oct 18, 2017

Note: Here is the last time we had - #58

@jasnell
Copy link
Member

jasnell commented Oct 18, 2017

This is awesome. Keep in mind that we're also running a small code-and-learn at NodeConf EU at the beginning of November so some of the tasks may adjust after that. We should likely coordinate a bit in advance to make sure we don't end up duplicating any effort! :-) (I totally wish I was able to get to Tokyo this year)

One possible set of tasks that I can suggest for new contributors who are bit more confident in their Node.js skills, would be converting tests to use the new ../common/countdown utility module.

For instance, if you take a look at: https://github.com/nodejs/node/blob/master/test/parallel/test-http2-client-destroy.js, you'll see that there is a remaining counter (https://github.com/nodejs/node/blob/master/test/parallel/test-http2-client-destroy.js#L19). The test then counts down remaining before closing the server. There are quite a large number of tests in our suite that perform similar actions in ways that are rather inconsistent. The ../common/countdown utility was designed to bring some consistency there.

The way the Countdown utility works is straightforward:

const common = require('../common');
const Countdown = require('../common/countdown');

// ...

const countdown = new Countdown(n, common.mustCall(() => {
  // do something here
}));

// Decrement the counter, the callback is called synchronously when
// countdown.dec is called n times.
countdown.dec();

I know that there are quite a few of the http2 tests that can benefit from this, along with a bunch of other http and https tests.

These tasks would be for folks who are a bit more comfortable with their Node.js skills.

@apapirovski
Copy link
Member

apapirovski commented Oct 18, 2017

Another potential suggestion is replacing assert.throws(fn, common.expectsError(err)); with common.expectsError(fn, err); It's another thing I would like to ideally introduce an eslint rule for (already have it written) but there are currently a ton of instances where we use the former.

I think that's pretty similar in difficulty to the change that was done at Node.js Interactive this year.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Oct 18, 2017

A simple thing is removing the redundant + from +conf.n in the benchmarks. If the values are integers in the bench(main, conf) object they will be integers in the main(conf) object too.

example: https://github.com/nodejs/node/blob/master/benchmark/assert/deepequal-object.js#L30L31

@hiroppy
Copy link
Member Author

hiroppy commented Oct 19, 2017

@jasnell @apapirovski @AndreasMadsen Thanks.🙌 I try to summarize that information in Gist.
You should come to Nodefest2017!!

@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 19, 2017 via email

@apapirovski
Copy link
Member

apapirovski commented Oct 19, 2017

@abouthiroppy For my suggestion above, I created a gist with an eslint rule that will let you find all instances of assert.throws(fn, common.expectsError(err));.

@hiroppy
Copy link
Member Author

hiroppy commented Oct 20, 2017

@MylesBorins Thanks!! yep, but I don't know the values session of node interactive. Would you give me this information?

@apapirovski wow, thank you so much 🙇 I'll use it!

@seishun
Copy link

seishun commented Oct 23, 2017

I'm on the fence about attending (concerned about the cost mostly).

@fhinkel
Copy link
Member

fhinkel commented Nov 11, 2017

Also, I will be there. Happy to help out with whatever needed.

@hiroppy
Copy link
Member Author

hiroppy commented Nov 18, 2017

Sorry for my late reply.

@seishun I think that Node.js Foundation will help with the cost.
@fhinkel Thanks!

@Trott
Copy link
Member

Trott commented Nov 18, 2017

@seishun I think that Node.js Foundation will help with the cost.

I don't think the foundation typically sends Collaborators to an event simply because it's a Node.js event. It's usually a Collaborator Summit. There have been some exceptions though, like sending particular people to a TC-39 meeting one time.

That said, there's absolutely nothing stopping any Collaborator from requesting funds from the TSC for something by opening an issue in the TSC repository.

@MylesBorins
Copy link
Contributor

Have we prepared a list of changes for participants to implement?

How many people are we expecting?

@yosuke-furukawa
Copy link
Member

I guess 30 - 50 people.

@yosuke-furukawa
Copy link
Member

Have we prepared a list of changes for participants to implement?
Not yet !! I will create the list asap...

@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 20, 2017 via email

@hiroppy
Copy link
Member Author

hiroppy commented Nov 20, 2017

a list of changes for participants 

TODO file line note status
Fix typo doc/api/util.md 695 occuring
Add link of ECMAScript 2015 doc/api/buffer.md 2740
Add link to dns.resolve() doc/api/dns.md 632
Use common.hasIntl instead of typeof Intl test/parallel/test-intl-v8BreakIterator.js 6
replace Function with Arrow Function doc/api/util.md 442, 552, 567, 568
test-writeint.js
test-querystring.js
test-assert.js
test-http.js 109, 118, 127, 128
test-http-pause.js
doc/api/vm.md 477
test-timers.js
test-whatwg-url-searchparams-getall.js
test-writeuint.js
test-child-process-send-cb.js
test-zerolengthbufferbug.js
test-domain-top-level-error-handler-clears-stack.js
use === and ' test-whatwg-url-setters.js 43
add ; test-whatwg-url-setters.js 35, 81
make use of const or let test-whatwg-url-setters.js 41 - 49
many files.... find ./ -type f -print | xargs grep var
fix comments test-assert.js 617, 5292
make use of Number.isNaN test-process-emit.js 22
test-readdouble.js
test-readfloat.js
test-writedouble.js
test-writefloat.js

please modify freely!

@joyeecheung
Copy link
Member

joyeecheung commented Nov 21, 2017

We can try replacing all the google.com or this.hostname.is.invalid etc. references in test/parallel with configurable variables defined in internet.addresses

@joyeecheung
Copy link
Member

joyeecheung commented Nov 21, 2017

Also there are a few tasks that could be more advanced: find the tests in test/parallel that are making actual dns calls, use mocked lookup functions or put those in test/internet. Searching for DNS error codes like EAI_AGAIN or ENOTFOUND in test/parallel would yield a bunch of tests that try to work around this even though they are not supposed to work around that.

@joyeecheung
Copy link
Member

There is also nodejs/node#17169 , see nodejs/node#17169 (comment)

@watilde
Copy link
Member

watilde commented Nov 23, 2017

I've added some to #72 (comment).

@joyeecheung
Copy link
Member

TODO file line note status
Replace e->ToObject(env->isolate()) with e->ToObject(env->context()).ToLocalChecked() src/node.cc line 592
Replace e->ToObject(env->isolate()) with e->ToObject(env->context()).ToLocalChecked() src/node.cc line 754
Replace er->ToObject(env->isolate()) with er->ToObject(env->context()).ToLocalChecked() src/node.cc line 1485
Replace args[0]->ToObject(env->isolate()) with args[0]->ToObject(env->context()).ToLocalChecked() src/node.cc line 2304
Replace args[1]->ToString(env->isolate()) with args[1]->ToString(env->context()).ToLocalChecked() src/node.cc line 2322
Replace module->Get(exports_string)->ToObject(env->isolate()) with module->Get(exports_string)->ToObject(env->context()).ToLocalChecked() src/node.cc line 2367
Replace process_object->Get(exit_code)->ToInteger(env->isolate()) with process_object->Get(exit_code)->ToInteger(env->context()).ToLocalChecked() src/node.cc line 4340
Replace args[1]->ToString(env->isolate()) with args[1]->ToString(env->context()).ToLocalChecked() src/node_buffer.cc line 609
Replace args[0]->ToString(env->isolate()) with args[0]->ToString(env->context()).ToLocalChecked() src/node_buffer.cc line 680
Replace args[0]->ToString(env->isolate()) with args[0]->ToString(env->context()).ToLocalChecked() src/node_contextify.cc line 624
Replace args[1]->ToObject(env->isolate()) with args[1]->ToObject(env->context()).ToLocalChecked() src/node_file.cc line 1222
Replace e->ToObject(env->isolate()) with e->ToObject(env->context()).ToLocalChecked() src/node_http_parser.cc line 469
Replace args[1]->ToObject(env->isolate()) with args[1]->ToObject(env->context()).ToLocalChecked() src/node_zlib.cc line 181
Replace args[4]->ToObject(env->isolate()) with args[4]->ToObject(env->context()).ToLocalChecked() src/node_zlib.cc line 190
Replace args[0]->ToObject(env->isolate()) with args[0]->ToObject(env->context()).ToLocalChecked() src/process_wrap.cc line 146
Replace chunk->ToString(env->isolate()) with chunk->ToString(env->context()).ToLocalChecked() src/stream_base.cc line 130
Replace chunk->ToString(env->isolate()) with chunk->ToString(env->context()).ToLocalChecked() src/stream_base.cc line 182

This is generated with a script but should be a safe list

@joyeecheung
Copy link
Member

Sorry, accidentally pressed the wrong button

@kt3k
Copy link

kt3k commented Nov 26, 2017

It was very fun! thank you!

@hiroppy
Copy link
Member Author

hiroppy commented Nov 26, 2017

@yosuke-furukawa @joyeecheung @MylesBorins Thank you for your help! I was really happy :)

@hiroppy hiroppy closed this as completed Nov 26, 2017
vsemozhetbyt pushed a commit to nodejs/node that referenced this issue Nov 29, 2017
1. Among the list of Code and Learn,
I solved the unfinished task of replacing function with arrow function:
nodejs/code-and-learn#72 (comment)

2. Replace arrow function with shorter property syntax
Arrow function makes `this` lexical scope.
But toString expects evaluate `this` in runtime.

3. Replace this with null
makeBlock does not need `this`.
update `this` with `null` to clarify the intent.

PR-URL: #17345
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 12, 2017
1. Among the list of Code and Learn,
I solved the unfinished task of replacing function with arrow function:
nodejs/code-and-learn#72 (comment)

2. Replace arrow function with shorter property syntax
Arrow function makes `this` lexical scope.
But toString expects evaluate `this` in runtime.

3. Replace this with null
makeBlock does not need `this`.
update `this` with `null` to clarify the intent.

PR-URL: #17345
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 12, 2017
1. Among the list of Code and Learn,
I solved the unfinished task of replacing function with arrow function:
nodejs/code-and-learn#72 (comment)

2. Replace arrow function with shorter property syntax
Arrow function makes `this` lexical scope.
But toString expects evaluate `this` in runtime.

3. Replace this with null
makeBlock does not need `this`.
update `this` with `null` to clarify the intent.

PR-URL: #17345
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
gibfahn pushed a commit to nodejs/node that referenced this issue Dec 19, 2017
1. Among the list of Code and Learn,
I solved the unfinished task of replacing function with arrow function:
nodejs/code-and-learn#72 (comment)

2. Replace arrow function with shorter property syntax
Arrow function makes `this` lexical scope.
But toString expects evaluate `this` in runtime.

3. Replace this with null
makeBlock does not need `this`.
update `this` with `null` to clarify the intent.

PR-URL: #17345
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
gibfahn pushed a commit to nodejs/node that referenced this issue Dec 20, 2017
1. Among the list of Code and Learn,
I solved the unfinished task of replacing function with arrow function:
nodejs/code-and-learn#72 (comment)

2. Replace arrow function with shorter property syntax
Arrow function makes `this` lexical scope.
But toString expects evaluate `this` in runtime.

3. Replace this with null
makeBlock does not need `this`.
update `this` with `null` to clarify the intent.

PR-URL: #17345
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
sreepurnajasti added a commit to sreepurnajasti/node that referenced this issue Jan 5, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 19, 2018
PR-URL: nodejs#17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
evanlucas pushed a commit to nodejs/node that referenced this issue Jan 30, 2018
PR-URL: #17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
evanlucas pushed a commit to nodejs/node that referenced this issue Jan 30, 2018
PR-URL: #17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Feb 27, 2018
PR-URL: #17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.