search

nodejs / code-and-learn

A series of workshop sprints for Node.js.
164 stars 79 forks source link

Taipei Nov 22 #77

Closed MylesBorins closed 6 years ago

MylesBorins commented 6 years ago

I'll be working with about 35 folks to get their first PRs in

Could use some help with generating content

/cc @Trott

We will be working on

Add missing common.crashOnUnhandledRejection: (all issues use promises or await)

Replace fs.accessSync with common.fileExists

Replace string concatenation with template literals

gireeshpunathil commented 6 years ago

Not sure these are good for code&learn items, but I would like to see:

fhinkel commented 6 years ago

@gireeshpunathil The only warnings at the moment are about v8::Debug functions being deprecated. Replacing those is probably not suited for a Code&Learn.

gireeshpunathil commented 6 years ago

Ah, ok. I double-checked the long list of warnings I usually get now, and see a number of warnings, but they are from SSL sources, which we don't have any control of. Thanks for clarifying!

MylesBorins commented 6 years ago

So here are the fixes I'm planning to run and the files they are related to. I only need around 35, so there should be some leftovers for tokyo. It is worth mentioning that there seems to be quite a bit of setTimeouts that are not using common.platformTimeout, perhaps there is a reason for this and I shouldn't get people to update all of these, but it seems like a good way to gather a non trivial number of changes for a larger event (such as tokyo).

Please let me know asap if you see any of these changes being problematic

Add missing common.crashOnUnhandledRejection: (all issues use promises or await)

Replace fs.accessSync with common.fileExists

Replace string concatenation with template literals

Trott commented 6 years ago

here seems to be quite a bit of setTimeouts that are not using common.platformTimeout,

Sorry, but please please please do not add common.platformTimeout(). It is an anti-pattern in most cases. Please, don't do it.

MylesBorins commented 6 years ago

Thanks for the heads up.

I'll drop those ones.

With that in mind I need about 10 more, any suggestions?

apapirovski commented 6 years ago

A simple one might be replacing assert.throws(fn, common.expectsError(err)); with common.expectsError(fn, err);. There are a ton of those and there's even an eslint rule to help find them: https://gist.github.com/apapirovski/b8d0803e6bd9938d29c207d50717eef3 — let me know if you need any help getting it setup.

Trott commented 6 years ago

Here's an easy one: I believe the remark-* directories in tools contain external code bases that we're linting. Add tools/remark-* to .eslintignore.

The following 33 files all have at least once case of string concatenation where template literals might be better. (You'll probably want to take a peek at the files to confirm that these aren't somehow non-trivial or cases where string concatenation really might be better. If you only need a few, stick to doc/test/tool directories to avoid perf discussions.)

benchmark/zlib/creation.js
doc/api/stream.md
doc/api/util.md
lib/_http_client.js
lib/_http_incoming.js
lib/_http_outgoing.js
lib/_tls_legacy.js
lib/_tls_wrap.js
lib/buffer.js
lib/child_process.js
lib/fs.js
lib/internal/child_process.js
lib/internal/cluster/master.js
lib/internal/errors.js
lib/internal/http2/core.js
lib/internal/loader/ModuleRequest.js
lib/internal/process/promises.js
lib/internal/querystring.js
lib/internal/streams/BufferList.js
lib/internal/trace_events_async_hooks.js
lib/internal/util.js
lib/internal/util/comparisons.js
lib/internal/v8_prof_processor.js
lib/module.js
lib/net.js
lib/os.js
lib/path.js
lib/querystring.js
lib/repl.js
lib/string_decoder.js
lib/tls.js
lib/url.js
lib/util.js
test/abort/test-zlib-invalid-internals-usage.js
test/async-hooks/init-hooks.js
test/parallel/test-assert.js
test/parallel/test-buffer-alloc.js
test/parallel/test-http-extra-response.js
test/parallel/test-http-multi-line-headers.js
test/parallel/test-http-parser.js
test/parallel/test-readline-interface.js
test/parallel/test-require-extensions-main.js
tools/doc/html.js
tools/doc/json.js
tools/license2rtf.js
MylesBorins commented 6 years ago

thanks @Trott added to original comment

Trott commented 6 years ago

I think you should totally go with them for tasks, but be aware that there's probably going to be some coaching needed a bit on the "use template literals rather than string concatenation" task. For example, in test/abort/test-zlib-invalid-internals-usage.js, there's this:

    'WARNING: You are likely using a version of node-tar or npm that ' +
    'is incompatible with this version of Node.js.' + os.EOL +
    'Please use either the version of npm that is bundled with Node.js, or ' +
    'a version of npm (> 5.5.1 or < 5.4.0) or node-tar (> 4.0.1) that is ' +
    'compatible with Node.js 9 and above.' + os.EOL

The concatenation across lines is totally fine. It's the concatenation within a line (the + os.EOL stuff) that needs to be replaced with a template literal. So when the contributor is done, there will be three lines that are untouched and are still string literals, and two lines that are converted to template literals.

You'll need to either explain that sort of thing in writing, or else just make sure all the mentors know the deal.

Worst case, though, is it comes back as a nit in the PR, so...¯\(ツ)/¯

MylesBorins commented 6 years ago

All done and lots of great PRs.

Thanks for helping with review y'all