esm: Add support for pjson cache and main without extensions #18728

guybedford · 2018-02-12T11:34:28Z

I've pulled out the refactorings from #18392 that aren't directly related to the package.json flag, in particular two features:

Ensuring that node --experimental-modules module-without-an-extension works as a special case. This is necessary for ensuring backwards-compatibility with NodeJS bin workflows. But extensions are still not permitted in dependencies.
Adding a package.json cache to the C++ resolver code. As mentioned previously the goal here is to ultimately share the cache with the CommonJS resolver, but for now at least this is a first step and better than having no cache as we do currently.

Review much appreciated especially with the C++ code.

Hopefully this shouldn't be too controversial allowing these useful features to not be blocked by progress on #18392.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

esmodules

This adds support for ensuring that the top-level main into Node is supported loading when it has no extension for backwards-compat with NodeJS bin workflows. In addition package.json caching is implemented in the module lookup process.

benjamingr · 2018-02-12T13:39:34Z

@nodejs/modules , also I missed you joining the collaborator ranks - so here's a late welcome :)

benjamingr · 2018-02-12T13:42:00Z

Any obvious reason this isn't two PRs (one for the main without extensions, one for the cache) by the way?

guybedford · 2018-02-12T13:50:41Z

This could just as well be two separate PRs, although there is some minor general refactoring here as well.

benjamingr

Very nice, would love adding some tests (not sure how to test the caching behavior though)

benjamingr · 2018-02-12T13:44:24Z

lib/internal/loader/DefaultResolve.js

    url.search = old.search;
    url.hash = old.hash;
  }

  const ext = extname(url.pathname);
-  return { url: `${url}`, format: extensionFormatMap[ext] || ext };
+
+  const format = extensionFormatMap[ext] || parentURL === undefined && 'cjs';


This line is confusing IMO, I'd write it as either an if/else or at least use parenthesis.

benjamingr · 2018-02-12T13:45:12Z

lib/internal/loader/DefaultResolve.js

+  if (!format)
+    throw new errors.Error('ERR_UNKNOWN_FILE_EXTENSION', url.pathname);
+
+  return { url: `${url}`, format };


Just wondering, why is this wrapping of ${url} required?

This is a URL -> string serialization convention in this code.

benjamingr · 2018-02-12T13:46:25Z

lib/internal/loader/Loader.js

-  async resolve(specifier, parentURL = this.base) {
-    if (typeof parentURL !== 'string')
+  async resolve(specifier, parentURL) {
+    if (parentURL !== undefined && typeof parentURL !== 'string')


I'm not sure I like the new logic (of using parentURL !== undefined instead of an explicit check) - If possible I'd at least make the check a getter or a function and call that instead.

Loaders exist in their own context, without even a this reference currently, so the alternative channel would be arguments. So the idea of parentURL === undefined is that it avoids the need for a breaking change in the arguments when we already have it, and it makes logical API sense in terms of the tree structure of modules.

Right, all I'm saying is that I'd likely put a function called isMain and pass parentURL to it to make it explicit - without the context of this PR it wouldn't be immediately obvious to me what it does

We might want to be a bit careful about over abstracting here if we ever want to support a --require like flag for ESM, which I presume also would not have a parentURL, but wouldn't be a "main"?

Sure, we could assign to isMain first to make this clearer.

benjamingr · 2018-02-12T13:50:49Z

lib/internal/process/modules.js

 function initializeImportMetaObject(wrap, meta) {
  meta.url = wrap.url;
 }

 function setupModules() {
  setInitializeImportMetaObjectCallback(initializeImportMetaObject);
+
+  let ESMLoader = new Loader();
+  const loaderPromise = (async () => {


This isn't a great pattern and errors here are suppressed unless explicitly asked for

benjamingr · 2018-02-12T13:51:33Z

lib/internal/process/modules.js

-};
+exports.setup = setupModules;
+exports.ESMLoader = undefined;
+exports.loaderPromise = undefined;


I'm not sure we want to start this as undefined, maybe set to a promise that we'd resolve with the "new" loaderPromise data?

Sure I'll switch around some of the async logic.

benjamingr · 2018-02-12T13:55:43Z

lib/module.js

-      await ESMLoader.import(getURLFromFilePath(request).pathname);
-    })()
+  if (experimentalModules && isMain) {
+    internalESModule.loaderPromise.then((loader) => {


I'm not a fan of our error handling logic here of console.error(e); process.exit(1) but that's unrelated to this PR. I might raise it at the team meeting.

@benjamingr that was done because unhandled rejection gave even worse error messages to the user.

@bmeck then we should either fix the unhandled rejection error messages or throw here in nextTick so that people with uncaughtException checks can intercept it if they can for synchronous loading?

If you can point me towards related reading I promise to read it :)

related reading??

For adding the behavior for error messages

#14369 (comment)

benjamingr · 2018-02-12T13:57:28Z

src/module_wrap.cc

@@ -494,23 +493,73 @@ Maybe<uv_file> CheckFile(const URL& search,
  return Just(fd);
 }

+PackageConfig emptyPackage = { false, false, "" };
+std::unordered_map<std::string, PackageConfig> pjson_cache_;


Are we OK with keeping this in memory for all time? (I think so, just checking, since this means more memory that's always alive in the process)

Yes it's a cache for the lifetime of the process and we're not maintaining the full contents just the main string.

benjamingr · 2018-02-12T13:58:14Z

src/module_wrap.cc

+  }
+  Maybe<uv_file> check = CheckFile(path, LEAVE_OPEN_AFTER_CHECK);
+  if (check.IsNothing()) {
+    return pjson_cache_[path] = emptyPackage;


I'd personally prefer it if this was split into two lines

Ditto for other usages of this pattern

benjamingr · 2018-02-12T14:00:28Z

src/module_wrap.cc

 enum ResolveExtensionsOptions {
  TRY_EXACT_NAME,
  ONLY_VIA_EXTENSIONS
 };

 template<ResolveExtensionsOptions options>
-Maybe<URL> ResolveExtensions(const URL& search) {
+Maybe<URL> ResolveExtensions(Environment* env, const URL& search) {


Why did you add env here? Is it used somewhere and I missed it?

Thanks, yes this is unused.

bmeck · 2018-02-12T14:17:15Z

src/module_wrap.cc

-  if (!ShouldBeTreatedAsRelativeOrAbsolutePath(main_std)) {
-    main_std.insert(0, "./");
+  if (!ShouldBeTreatedAsRelativeOrAbsolutePath(pjson.main)) {
+    return Resolve(env, "./" + pjson.main, search);


RESOLVED: DELEGATE TO SEPARATE PR

Does this get weird with a leading /? I'm not entirely sure what this is trying to do versus below.

This is just a rewriting of the original:

if (!ShouldBeTreatedAsRelativeOrAbsolutePath(main_std)) { main_std.insert(0, "./"); } return Resolve(env, main_std, search);

into:

if (!ShouldBeTreatedAsRelativeOrAbsolutePath(main_std)) { return Resolve(env, "./" + pjson.main, search); } return Resolve(env, main_std, search);

which I believe is invariant. But happy to revert otherwise too.

this looks like a breaking bug that I introduced a long time ago from CJS when I dug up git blame. I'll deal with this in a separate PR.

Glad to have helped surface. Ideally I think the pjson.main should be pre-sanitized at the cache phase, so that that can already be handled by the time the code gets here.

guybedford · 2018-02-12T14:26:36Z

@benjamingr thanks for the review. The test at https://github.com/nodejs/node/pull/18728/files#diff-d3da74a6bdb693be276bdef1df06123fL7 catches the main extension case, while the rest of the module tests will all use the cache (there are repeat loads of the same package already that would apply here).

benjamingr · 2018-02-12T14:38:38Z

while the rest of the module tests will all use the cache (there are repeat loads of the same package already that would apply here).

Yeah, though technically they're not testing that code - as in: the code could cache or not cache and the tests wouldn't wouldn't fail. Although I'm not sure how to test it or even if we should.

benjamingr

Second round

benjamingr · 2018-02-12T14:39:35Z

doc/api/esm.md


 export async function resolve(specifier, parentModuleURL, defaultResolver) {
  return {
-    url: new URL(specifier, parentModuleURL).href,
+    url: new URL(specifier, parentModuleURL || baseURL).href,


Given we use the pattern parentModuleURL = baseURL below for the default argument value - it's probably a good idea to stay consistent in the docs here and use it too.

benjamingr · 2018-02-12T14:39:48Z

doc/api/esm.md

    format: 'esm'
  };
 }
 ```

+The parentURL is provided as `undefined` when performing main NodeJS load itself.


nit: NodeJS -> Node.js

benjamingr · 2018-02-12T14:42:19Z

lib/internal/loader/Loader.js

-        inspectBrk = true;
-      }
-      this.isMain = false;
+    if (process._breakFirstLine) {


Reading this again, can you explain why removing the if (this.main) without if (parentURL) is valid?

Because we clear _breakFirstLine it's guaranteed to only trigger for the first module loaded.

benjamingr · 2018-02-12T14:45:01Z

lib/internal/process/modules.js

-  setInitializeImportMetaObjectCallback(initializeImportMetaObject);
-}
+exports.loaderPromise = new Promise((resolve, reject) => {
+  exports.setup = function() {


This works (defining setup inside the promise) but has some side effects - for example synchronous throws from setup will now be converted to loaderPromise errors.

I'd do something like:

let loaderPromiseResolve; exports.loaderPromise = new Promise(resolve => { loaderPromiseResolve = resolve; });

And then define loaderPromise as resolve, and use loaderPromiseResolve on the new loaderPromise (since you can pass a rejected promise to it and it'll reject the original promise).

benjamingr

A few more nits about the promises - overall lgtm

benjamingr · 2018-02-12T15:04:59Z

lib/internal/process/modules.js

 function initializeImportMetaObject(wrap, meta) {
  meta.url = wrap.url;
 }

-function setupModules() {
+let loaderResolve, loaderReject;


So, elaborating on what I wrote before:

let aResolve, aReject; exports.loaderPromise = new Promise((resolve, reject) => { aResolve = resolve; aReject = reject; }); trackedPromise.then(aResolve, aReject);

Can be written as:

let aResolve; exports.loaderPromise = new Promise((resolve, reject) => { aResolve = resolve; }); aResolve(trackedPromise);

could also just use createPromise/promiseReject/promiseResolve, no?

@devsnek that's technically true but I'm not sure it would help clean the code and that's an internal API - I doubt the cost of the extra closure here is worth it.

I haven't looked through this code at all, so take this with a grain of salt, but I recently did a lot of benchmarking on using the Promise constructor vs the internal createPromise/promiseReject/promiseResolve and the latter is definitely quite a bit faster.

But again, I don't know about the usage here, whether this is performance sensitive code, if these functions end up being user-land exposed, etc.

i just meant for readability heh, i was actually under the impression that the api was slower than the promise constructor

const loaderPromise = exports.loaderPromise = createPromise(); ... promiseResolve(loaderPromise, ESMLoader);

@devsnek @apapirovski not allocating a closure and it being faster is actually why we have this API to begin with (as well as util.promisify). There's a bit of history involved but if it wasn't faster I'd be surprised. That said - this code does I/O and the impact of an extra closure isn't something we should optimize for.

@devsnek I think it makes the code easier for people more familiar with promises in core to read but overall I personally prefer standard to nonstandard when we have the choice.

Namely, there are subtle differences between promiseResolve and resolve in the promise constructor which I'd rather avoid in areas that are not performance sensitive. I don't feel strongly about it though.

benjamingr · 2018-02-12T15:06:47Z

lib/module.js

-      await ESMLoader.import(getURLFromFilePath(request).pathname);
-    })()
+  if (experimentalModules && isMain) {
+    internalESModule.loaderPromise.then((loader) => {


@bmeck then we should either fix the unhandled rejection error messages or throw here in nextTick so that people with uncaughtException checks can intercept it if they can for synchronous loading?

If you can point me towards related reading I promise to read it :)

benjamingr

LGTM, would love it if more people gave this a look and made sure this behaves as expected and there are no subtle C++ issues I missed

bnoordhuis · 2018-02-12T15:43:26Z

src/module_wrap.cc

@@ -494,6 +493,59 @@ Maybe<uv_file> CheckFile(const URL& search,
  return Just(fd);
 }

+PackageConfig emptyPackage = { false, false, "" };


static const PackageConfig kEmptyPackage = ... and since it's only used in GetPackageConfig() move it into that function.

bnoordhuis · 2018-02-12T15:45:09Z

src/module_wrap.cc

@@ -494,6 +493,59 @@ Maybe<uv_file> CheckFile(const URL& search,
  return Just(fd);
 }

+PackageConfig emptyPackage = { false, false, "" };
+std::unordered_map<std::string, PackageConfig> pjson_cache_;


Style: pjson_cache, no trailing underscore. package_json_cache is a better name, IMO.

Substance: should be a member of Environment.

bnoordhuis · 2018-02-12T15:46:24Z

src/module_wrap.cc

+  Maybe<uv_file> check = CheckFile(path, LEAVE_OPEN_AFTER_CHECK);
+  if (check.IsNothing()) {
+    return pjson_cache_[path] =
+           emptyPackage;


No linebreak, fits on one line. Likewise below.

bnoordhuis · 2018-02-12T15:48:40Z

src/module_wrap.cc

+  }
+
+  Isolate* isolate = env->isolate();
+  Local<Context> context = isolate->GetCurrentContext();


env->context()? You should probably create a v8::HandleScope handle_scope(isolate) first since you're creating JS handles.

bnoordhuis · 2018-02-12T15:49:01Z

src/module_wrap.cc

+  Local<Context> context = isolate->GetCurrentContext();
+  std::string pkg_src = ReadFile(check.FromJust());
+  uv_fs_t fs_req;
+  uv_fs_close(nullptr, &fs_req, check.FromJust(), nullptr);


Check the return value.

This is actually the same code we have got currently just moved around a bit.

I think to properly do this would involve improving the error handling here, which I may not be the best person for as I'm not sure what mechanisms would be best here without further pointers.

Since I wouldn't expect it to fail, a simple CHECK should suffice:

CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, check.FromJust(), nullptr));

bnoordhuis · 2018-02-12T15:50:44Z

src/module_wrap.cc

+  }
+
+  Local<Value> pkg_json;
+  if (!JSON::Parse(context, src).ToLocal(&pkg_json) || !pkg_json->IsObject())


Can you use the ToObject() overload that takes a Context?

I'm' not sure I follow exactly what is intended here. Do you mean something like:

Local<Value> pkg_json = JSON::Parse(context, src).ToLocalChecked(); MaybeLocal<Object> pkg_json_obj = pkg_json->ToObject(context); if (pkg_json_obj.isEmpty()) // ...

You probably don't want to use .ToLocalChecked() on the return value of JSON::Parse() because that will abort node when the JSON is malformed without a clear error message (aside: that's also an argument for removing the TryCatch; the exception should bubble up.)

Essentially, you'd do something like this:

Local<Value> package_json_v; Local<Object> package_json; if (!JSON::Parse(context, src).ToLocal(&package_json_v)) return; // Exception pending. if (!package_json_v->ToObject(context).ToLocal(&package_json)) return; // Exception pending. // |package_json| points to a valid JS object now.

bnoordhuis · 2018-02-12T15:52:50Z

src/module_wrap.cc

@@ -594,8 +618,8 @@ Maybe<URL> ResolveModule(Environment* env,

 Maybe<URL> ResolveDirectory(Environment* env,
                            const URL& search,
-                            bool read_pkg_json) {
-  if (read_pkg_json) {
+                            bool check_pjson_main) {


Ideally, use an enum, not a bool; easier to read at call sites.

bnoordhuis · 2018-02-12T15:53:32Z

src/module_wrap.h

+struct PackageConfig {
+  bool exists;
+  bool has_main;
+  std::string main;


Make these const if at all possible.

I couldn't get this to work unfortunately - seems to be a result of PackageConfig not being a pointer on the map itself in the assignment operator.

What was the error you got?

These two:

error: use of deleted function ‘node::Environment::PackageConfig& node::Environment::PackageConfig::operator=(const node::Environment::PackageConfig&)’ error: non-static const member ‘const bool node::Environment::PackageConfig::exists’, can’t use default assignment operator

@guybedford that's just because since it's const it must be fully intiialized - when you do myMap['foo'] = bar that potentially creates an empty object and then assigns bar to it.

You can use emplace to construct the object inside the map to overcome it.

bnoordhuis · 2018-02-12T15:55:10Z

src/module_wrap.cc

+  PackageConfig pjson = { true, has_main, main_std };
+  return pjson_cache_[path] =
+         pjson;
+}


Return either a const PackageConfig& or a PackageConfig*. Mutable references are tricky because it's not always clear at the call site if you're manipulating the original or a copy.

It's not shown here, but this issue is addressed and const PackageConfig& is returned.

guybedford · 2018-02-12T21:16:32Z

@bnoordhuis thanks for the feedback, I've included the points as mentioned, perhaps just check I've got these along the right lines.

bnoordhuis · 2018-02-12T21:29:45Z

src/env.h

+    bool exists;
+    bool has_main;
+    std::string main;
+  };


PackageConfig should probably live in the node::loader namespace, like ModuleWrap.

bnoordhuis · 2018-02-12T21:31:09Z

src/module_wrap.cc

+    return kEmptyPackage;
+
+  Isolate* isolate = env->isolate();
+


Teeniest of style nits: can you drop the blank line?

bnoordhuis · 2018-02-12T21:32:15Z

src/module_wrap.cc

+
+  // It's not okay for the called of this method to not be able to tell
+  // whether an exception is pending or not.
+  TryCatch try_catch(isolate);


The TryCatch means exceptions from the code below won't propagate. Either remove this (I think you should) or drop the 'exception pending' comments below.

So what we'd actually want is to throw a JS error for JSON parsing failure. But if the above is definitely not going to help with that sure, it can be removed.

bnoordhuis · 2018-02-12T21:32:31Z

src/module_wrap.cc

+
+  if (!JSON::Parse(env->context(), src).ToLocal(&pkg_json_v) ||
+      !pkg_json_v->ToObject(env->context()).ToLocal(&pkg_json))
+    return kEmptyPackage;  // Exception pending.


Can you put braces around the consequent?

bnoordhuis · 2018-02-12T21:36:32Z

src/module_wrap.cc

+  if (pkg_json->Get(env->context(), env->main_string())
+                              .ToLocal(&pkg_main) && pkg_main->IsString()) {
+    has_main = true;
+    Utf8Value main_utf8(isolate, pkg_main.As<String>());


.As<String>() isn't necessary. You don't per se need the IsString() check either, Utf8Value will coerce the value to string.

bnoordhuis · 2018-02-12T21:36:35Z

src/module_wrap.cc

+                              .ToLocal(&pkg_main) && pkg_main->IsString()) {
+    has_main = true;
+    Utf8Value main_utf8(isolate, pkg_main.As<String>());
+    main_std = std::string(*main_utf8, main_utf8.length());


Maybe use main_std.assign() here. The compiler will probably elide the temporary but .assign() will for sure.

bnoordhuis · 2018-02-12T21:41:32Z

src/module_wrap.h

@@ -12,10 +12,15 @@
 namespace node {
 namespace loader {

+enum package_main_check : bool {


Style: should be something like PackageMainCheck or CheckPackageMain.

bnoordhuis · 2018-02-12T21:42:42Z

src/env.h

+    bool is_valid;
+    bool has_main;
+    std::string main;
+  };


Would still be nice if these were const.

As far as I can tell getting this to work would mean inlining the kEmptyPackage constructor at each place it is assigned through an emplace call, which doesn't seem very nice.

(as in kEmptyPackage would be replaced by these inline constructors?)

map.emplace(std::make_pair(key, kEmptyPackage)) should work, I think. If it doesn't, you could add a PackageConfig() = default constructor and emplace PackageConfig() instead.

The first case didn't work unfortunately. In the second case it would need to be a PackageConfig(true) and a PackageConfig(false) to avoid a full expansion of arguments. But = default doesn't seem to be compatible with default constructor arguments as far as I can tell?

I managed to get the consts to work out, but it's pretty awful code to look at.

PR-URL: nodejs#18788 Refs: nodejs#18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-02-21T14:49:16Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

needs to come with #18788

guybedford · 2018-02-21T21:52:16Z

It would be nice to get it in, as backwards compatibility for Node bins is an important use case I feel. Will work on the backport here first thing tomorrow.

guybedford · 2018-02-21T23:52:50Z

Created #18923 for the backport.

guybedford · 2018-02-22T10:29:42Z

(To note the backport issue - this should land after #18368)

This adds support for ensuring that the top-level main into Node is supported loading when it has no extension for backwards-compat with NodeJS bin workflows. In addition package.json caching is implemented in the module lookup process. PR-URL: nodejs#18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs#18788 Refs: nodejs#18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds support for ensuring that the top-level main into Node is supported loading when it has no extension for backwards-compat with NodeJS bin workflows. In addition package.json caching is implemented in the module lookup process. Backport-PR-URL: #18923 PR-URL: #18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Backport-PR-URL: #18923 PR-URL: #18788 Refs: #18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds support for ensuring that the top-level main into Node is supported loading when it has no extension for backwards-compat with NodeJS bin workflows. In addition package.json caching is implemented in the module lookup process. PR-URL: nodejs#18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs#18788 Refs: nodejs#18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-08-07T17:46:40Z

Should this be backported to 8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

MylesBorins · 2018-08-07T18:38:56Z

needs to come with #18788 if backported

guybedford · 2018-08-07T18:57:50Z

@MylesBorins for #18788, can you try pulling through #18923? Or should I make this up again against v8.x?

MylesBorins · 2018-08-07T22:22:48Z

@guybedford that one doesn't land cleanly either... we may need a bit of a sprint to catch the esm implementation in 8.x caught up

guybedford · 2018-08-09T10:34:32Z

@MylesBorins sure will aim to get to this when I have some time.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 12, 2018

guybedford added the esm Issues and PRs related to the ECMAScript Modules implementation. label Feb 12, 2018

guybedford force-pushed the esm-resolve-refactor-cache branch 3 times, most recently from 9b71cb0 to 319a22d Compare February 12, 2018 12:04

guybedford force-pushed the esm-resolve-refactor-cache branch from 319a22d to f14420e Compare February 12, 2018 13:03

benjamingr reviewed Feb 12, 2018

View reviewed changes

bmeck reviewed Feb 12, 2018

View reviewed changes

pr feedback

adfe1ac

explicit isMain checks

ebdb3cb

benjamingr reviewed Feb 12, 2018

View reviewed changes

loader setup promise tweaks

5855e5c

benjamingr reviewed Feb 12, 2018

View reviewed changes

further tweaks

05c9941

benjamingr approved these changes Feb 12, 2018

View reviewed changes

bnoordhuis reviewed Feb 12, 2018

View reviewed changes

c++ feedback

2d6d4e6

guybedford force-pushed the esm-resolve-refactor-cache branch from d007ba1 to 2d6d4e6 Compare February 12, 2018 21:15

better todo for JSON parse failure

a06db2d

guybedford force-pushed the esm-resolve-refactor-cache branch from 7330c86 to 717d4ca Compare February 12, 2018 21:36

invalid json handling todo

081aab4

guybedford force-pushed the esm-resolve-refactor-cache branch from 717d4ca to 081aab4 Compare February 12, 2018 21:39

bnoordhuis reviewed Feb 12, 2018

View reviewed changes

guybedford mentioned this pull request Feb 14, 2018

module: fix main lookup regression from #18728 #18788

Closed

3 tasks

guybedford added a commit to guybedford/node that referenced this pull request Feb 14, 2018

module: fix main lookup regression from nodejs#18728

3e9066c

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018

module: fix main lookup regression from nodejs#18728

7748865

PR-URL: nodejs#18788 Refs: nodejs#18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

guybedford mentioned this pull request Feb 21, 2018

[v9.x backport] module: main w/o extension, pjson cache (#18728, #18788) #18923

Closed

2 tasks

harrysarson mentioned this pull request Feb 26, 2018

WIP: use dynamic import to load es module tests. mochajs/mocha#3253

Closed

guybedford added a commit to guybedford/node that referenced this pull request Mar 4, 2018

module: fix main lookup regression from nodejs#18728

a366599

PR-URL: nodejs#18788 Refs: nodejs#18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 7, 2018

module: fix main lookup regression from #18728

39e032f

Backport-PR-URL: #18923 PR-URL: #18788 Refs: #18728 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Mar 7, 2018

v9.8.0 proposal #19181

Merged

MylesBorins added backport-requested-v8.x and removed backport-requested-v9.x labels Aug 7, 2018

esm: Add support for pjson cache and main without extensions #18728

esm: Add support for pjson cache and main without extensions #18728

Conversation

guybedford commented Feb 12, 2018

Checklist

Affected core subsystem(s)

benjamingr commented Feb 12, 2018 • edited Loading

benjamingr commented Feb 12, 2018

guybedford commented Feb 12, 2018

benjamingr 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

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

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

bmeck Feb 12, 2018 • 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

guybedford commented Feb 12, 2018

benjamingr commented Feb 12, 2018

benjamingr 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

Choose a reason for hiding this comment

benjamingr 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

devsnek Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Feb 12, 2018

Choose a reason for hiding this comment

benjamingr commented Feb 12, 2018 •

edited

Loading

bmeck Feb 12, 2018 •

edited

Loading

devsnek Feb 12, 2018 •

edited

Loading