src: improve module loader readability #16536

addaleax · 2017-10-26T23:48:13Z

Various improvements on readability, performance and
conformity to the Node core coding style in the ESM loader C++ code:

isolate for the Isolate*, context for the Local<Context>
Less reliance on auto where it’s unnecessary/increases cognitive
overhead
Shorter paths to failure via .ToLocal() & co
Do not keep pending exceptions around e.g. for failed JSON parsing
Use Maybe types to get explicit error status forwarding
Remove an unnecessary handle scope
Add nullptr checks for unwrapped host objects
Remove unused code
Use CamelCase for function names
Use const Foo& instead of copying values whenever possible
Pass along the Environment* explicitly

Checklist

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

Affected core subsystem(s)

src

addaleax · 2017-10-26T23:50:29Z

btw, re:

Use Maybe types to get explicit error status forwarding

… I realize this is still something a lot of people aren’t familiar with, but we’re stuck with that anyway because of V8. So see this as “you only have to understand one error forwarding system instead of two”, not as “this solves all problems & everybody gets a unicorn”. :)

guybedford

Looks great to me. It would be interesting as well to start getting some performance benchmarks going on this code too in due course.

guybedford · 2017-10-27T10:31:05Z

src/module_wrap.cc

-static URL INITIAL_CWD(__init_cwd());
-inline bool is_relative_or_absolute_path(std::string specifier) {
+// Tests whether a path starts with /, ./ or ../
+inline bool PathIsBasedOffCurrentDirectory(const std::string& specifier) {


Note that this is a URL-related function, designed to mimic the resolution steps under https://html.spec.whatwg.org/#integration-with-the-javascript-module-system.

I think isRelativeOrAbsolute remains a good name here.

Also it seems like you're flipping the polarity of this function? If that is the case perhaps a better name might be isBareSpecifier to match the WhatWG terminology.

Although since this function relies on parseAsURL already being checked, isNotRelativeOrAbsolute may be the most descriptive!

@guybedford I can’t find what exactly you’re referring to in that spec (also, linking to that page is a great way to DoS somebody’s browser)

At least outside of web specs, relative paths are the paths that are not absolute, so IsRelativeOrAbsolute really is not a helpful name?

Maybe IsBareSpecifier works, but even that isn’t really saying anything about what’s meant by it … I can add a comment indicating what this matches in WhatWG terminology, but the part of the goal of this change is to make it more accessible to people who don’t know all the spec phrases.

@addaleax here's the appropriate spec text, which is what is exactly what the function here is distinguishing:

To resolve a module specifier given a script script and a JavaScript string specifier, perform the following steps. It will return either a URL record or failure.

Apply the URL parser to specifier. If the result is not failure, return the result.

If specifier does not start with the character U+002F SOLIDUS (/), the two-character sequence U+002E FULL STOP, U+002F SOLIDUS (./), or the three-character sequence U+002E FULL STOP, U+002E FULL STOP, U+002F SOLIDUS (../), return failure and abort these steps.

This restriction is in place so that in the future we can allow custom module loaders to give special meaning to "bare" import specifiers, like import "jquery" or import "web/crypto". For now any such imports will fail, instead of being treated as relative URLs.

Return the result of applying the URL parser to specifier with script's base URL as the base URL.

IsNotRelativeOrAbsolute is trying to capture the concept of what is remaining being a plain or bare name (eg lodash).

Note that for example, we do not check .\ or handle internal .. segments, we are exactly just checking the above features. That is "bare names" are detected identically to the WhatWG URL spec, which then get resolved as packages.

It's nits though certainly.

Yeah, but this bikeshedding might be worth it. :)

So how does something like ShouldBeTreatedAsRelativeOrAbsolutePath sound to you? Does that capture the essence of the semantics well?

That sounds about right, yes. Although with the function polarity change, I think it would make it something like ShouldNotBeTreatedAsRelativeOrAbsolutePath? My comment below then may make more sense as well in terms of it being that confusing concept to humans of the double negative :)

I mean, we can just change polarity back … ;)

guybedford · 2017-10-27T10:36:49Z

src/module_wrap.cc

  }
-  if (is_relative_or_absolute_path(specifier)) {
+  if (!PathIsBasedOffCurrentDirectory(specifier)) {


This may be slightly easier to read by reversing the branch order instead of using negation:

if (IsBareSpecifier(specifier)) { return ResolveModule(env, specifier, base); } else { // ... }

tniessen · 2017-10-27T11:39:49Z

src/node_url.h

+  URL(URL&&) = default;
+  URL& operator=(URL&&) = default;
+
+  URL() : URL("") {}


I would prefer a blank line before private members (no idea whether we lint this?).

Yeah, the linter complained, I just overlooked it. (I run into this particular one about 20 times a day … I’ll never get used to it 😄)

tniessen · 2017-10-27T11:41:23Z

src/module_wrap.cc

  args.GetReturnValue().Set(that);
 }

 void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
  Environment* env = Environment::GetCurrent(args);
-  Isolate* iso = args.GetIsolate();
-  EscapableHandleScope handle_scope(iso);


I think this makes using v8::EscapableHandleScope obsolete.

Huh. I thought the linter would catch that.

gireeshpunathil · 2017-10-28T05:11:07Z

src/module_wrap.cc

  ModuleWrap* obj = Unwrap<ModuleWrap>(that);
+  CHECK_NE(obj, nullptr);
  auto mod_context = that->CreationContext();


don't we have an appropriate Local abstraction for this auto?

Yup, thanks for pointing it out :)

Various improvements on readability, performance and conformity to the Node core coding style in the ESM loader C++ code: - `isolate` for the `Isolate*`, `context` for the `Local<Context>` - Less reliance on `auto` where it’s unnecessary/increases cognitive overhead - Shorter paths to failure via `.ToLocal()` & co - Do not keep pending exceptions around e.g. for failed `JSON` parsing - Use `Maybe` types to get explicit error status forwarding - Remove an unnecessary handle scope - Add `nullptr` checks for unwrapped host objects - Remove unused code - Use `CamelCase` for function names - Use `const Foo&` instead of copying values whenever possible - Pass along the `Environment*` explicitly

addaleax · 2017-10-28T12:30:32Z

CI: https://ci.nodejs.org/job/node-test-commit/13547/

addaleax · 2017-11-03T00:18:47Z

Landed in e22a8f1

Various improvements on readability, performance and conformity to the Node core coding style in the ESM loader C++ code: - `isolate` for the `Isolate*`, `context` for the `Local<Context>` - Less reliance on `auto` where it’s unnecessary/increases cognitive overhead - Shorter paths to failure via `.ToLocal()` & co - Do not keep pending exceptions around e.g. for failed `JSON` parsing - Use `Maybe` types to get explicit error status forwarding - Remove an unnecessary handle scope - Add `nullptr` checks for unwrapped host objects - Remove unused code - Use `CamelCase` for function names - Use `const Foo&` instead of copying values whenever possible - Pass along the `Environment*` explicitly PR-URL: #16536 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

Various improvements on readability, performance and conformity to the Node core coding style in the ESM loader C++ code: - `isolate` for the `Isolate*`, `context` for the `Local<Context>` - Less reliance on `auto` where it’s unnecessary/increases cognitive overhead - Shorter paths to failure via `.ToLocal()` & co - Do not keep pending exceptions around e.g. for failed `JSON` parsing - Use `Maybe` types to get explicit error status forwarding - Remove an unnecessary handle scope - Add `nullptr` checks for unwrapped host objects - Remove unused code - Use `CamelCase` for function names - Use `const Foo&` instead of copying values whenever possible - Pass along the `Environment*` explicitly PR-URL: nodejs#16536 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

Various improvements on readability, performance and conformity to the Node core coding style in the ESM loader C++ code: - `isolate` for the `Isolate*`, `context` for the `Local<Context>` - Less reliance on `auto` where it’s unnecessary/increases cognitive overhead - Shorter paths to failure via `.ToLocal()` & co - Do not keep pending exceptions around e.g. for failed `JSON` parsing - Use `Maybe` types to get explicit error status forwarding - Remove an unnecessary handle scope - Add `nullptr` checks for unwrapped host objects - Remove unused code - Use `CamelCase` for function names - Use `const Foo&` instead of copying values whenever possible - Pass along the `Environment*` explicitly PR-URL: #16536 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 26, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 26, 2017

addaleax force-pushed the esm-readability-c++ branch from 8eed88f to e8f3e1e Compare October 26, 2017 23:52

Qard approved these changes Oct 27, 2017

View reviewed changes

addaleax force-pushed the esm-readability-c++ branch from e8f3e1e to 9b95c92 Compare October 27, 2017 01:03

jasnell approved these changes Oct 27, 2017

View reviewed changes

guybedford reviewed Oct 27, 2017

View reviewed changes

tniessen approved these changes Oct 27, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 28, 2017

View reviewed changes

gireeshpunathil reviewed Oct 28, 2017

View reviewed changes

addaleax force-pushed the esm-readability-c++ branch from 6c1c410 to b619e41 Compare October 28, 2017 12:29

addaleax closed this Nov 3, 2017

addaleax deleted the esm-readability-c++ branch November 3, 2017 00:18

cjihrig mentioned this pull request Nov 6, 2017

v9.1.0 proposal #16851

Merged

MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Nov 14, 2017

gibfahn added the land-on-v8.x label Nov 14, 2017

gibfahn mentioned this pull request Nov 21, 2017

v8.9.2 proposal #17204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: improve module loader readability #16536

src: improve module loader readability #16536

addaleax commented Oct 26, 2017 •

edited

Loading

addaleax commented Oct 26, 2017

guybedford left a comment

guybedford Oct 27, 2017

guybedford Oct 27, 2017

guybedford Oct 27, 2017

addaleax Oct 27, 2017

guybedford Oct 27, 2017 •

edited

Loading

addaleax Oct 27, 2017

guybedford Oct 27, 2017

addaleax Oct 27, 2017

guybedford Oct 27, 2017

tniessen Oct 27, 2017

addaleax Oct 27, 2017

tniessen Oct 27, 2017

addaleax Oct 27, 2017

gireeshpunathil Oct 28, 2017

addaleax Oct 28, 2017

addaleax commented Oct 28, 2017

addaleax commented Nov 3, 2017

src: improve module loader readability #16536

src: improve module loader readability #16536

Conversation

addaleax commented Oct 26, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax commented Oct 26, 2017

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Oct 28, 2017

addaleax commented Nov 3, 2017

addaleax commented Oct 26, 2017 •

edited

Loading

guybedford Oct 27, 2017 •

edited

Loading