Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 13 commits into from
16 changes: 12 additions & 4 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,22 @@ The resolve hook returns the resolved file URL and module format for a
given module specifier and parent file URL:

```js
import url from 'url';
const baseURL = new URL('file://');
baseURL.pathname = process.cwd() + '/';

export async function resolve(specifier, parentModuleURL, defaultResolver) {
export async function resolve(specifier,
parentModuleURL = baseURL,
defaultResolver) {
return {
url: new URL(specifier, parentModuleURL).href,
format: 'esm'
};
}
```

The default NodeJS ES module resolution function is provided as a third
The parentURL is provided as `undefined` when performing main Node.js load itself.

The default Node.js ES module resolution function is provided as a third
argument to the resolver for easy compatibility workflows.

In addition to returning the resolved file URL value, the resolve hook also
Expand Down Expand Up @@ -155,7 +160,10 @@ import Module from 'module';
const builtins = Module.builtinModules;
const JS_EXTENSIONS = new Set(['.js', '.mjs']);

export function resolve(specifier, parentModuleURL/*, defaultResolve */) {
const baseURL = new URL('file://');
baseURL.pathname = process.cwd() + '/';

export function resolve(specifier, parentModuleURL = baseURL, defaultResolve) {
if (builtins.includes(specifier)) {
return {
url: specifier,
Expand Down
21 changes: 16 additions & 5 deletions lib/internal/loader/DefaultResolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

const { URL } = require('url');
const CJSmodule = require('module');
const internalURLModule = require('internal/url');
const internalFS = require('internal/fs');
const NativeModule = require('native_module');
const { extname } = require('path');
Expand All @@ -11,6 +10,7 @@ const preserveSymlinks = !!process.binding('config').preserveSymlinks;
const errors = require('internal/errors');
const { resolve: moduleWrapResolve } = internalBinding('module_wrap');
const StringStartsWith = Function.call.bind(String.prototype.startsWith);
const { getURLFromFilePath, getPathFromURL } = require('internal/url');

const realpathCache = new Map();

Expand Down Expand Up @@ -57,7 +57,8 @@ function resolve(specifier, parentURL) {

let url;
try {
url = search(specifier, parentURL);
url = search(specifier,
parentURL || getURLFromFilePath(`${process.cwd()}/`).href);
} catch (e) {
if (typeof e.message === 'string' &&
StringStartsWith(e.message, 'Cannot find module'))
Expand All @@ -66,17 +67,27 @@ function resolve(specifier, parentURL) {
}

if (!preserveSymlinks) {
const real = realpathSync(internalURLModule.getPathFromURL(url), {
const real = realpathSync(getPathFromURL(url), {
[internalFS.realpathCacheKey]: realpathCache
});
const old = url;
url = internalURLModule.getURLFromFilePath(real);
url = getURLFromFilePath(real);
url.search = old.search;
url.hash = old.hash;
}

const ext = extname(url.pathname);
return { url: `${url}`, format: extensionFormatMap[ext] || ext };

let format = extensionFormatMap[ext];
if (!format) {
const isMain = parentURL === undefined;
if (isMain)
format = 'cjs';
else
throw new errors.Error('ERR_UNKNOWN_FILE_EXTENSION', url.pathname);
}

return { url: `${url}`, format };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

module.exports = resolve;
Expand Down
59 changes: 11 additions & 48 deletions lib/internal/loader/Loader.js
Original file line number Diff line number Diff line change
@@ -1,51 +1,21 @@
'use strict';

const path = require('path');
const { getURLFromFilePath, URL } = require('internal/url');
const errors = require('internal/errors');

const ModuleMap = require('internal/loader/ModuleMap');
const ModuleJob = require('internal/loader/ModuleJob');
const defaultResolve = require('internal/loader/DefaultResolve');
const createDynamicModule = require('internal/loader/CreateDynamicModule');
const translators = require('internal/loader/Translators');
const { setImportModuleDynamicallyCallback } = internalBinding('module_wrap');

const FunctionBind = Function.call.bind(Function.prototype.bind);

const debug = require('util').debuglog('esm');

// Returns a file URL for the current working directory.
function getURLStringForCwd() {
try {
return getURLFromFilePath(`${process.cwd()}/`).href;
} catch (e) {
e.stack;
// If the current working directory no longer exists.
if (e.code === 'ENOENT') {
return undefined;
}
throw e;
}
}

function normalizeReferrerURL(referrer) {
if (typeof referrer === 'string' && path.isAbsolute(referrer)) {
return getURLFromFilePath(referrer).href;
}
return new URL(referrer).href;
}

/* A Loader instance is used as the main entry point for loading ES modules.
* Currently, this is a singleton -- there is only one used for loading
* the main module and everything in its dependency graph. */
class Loader {
constructor(base = getURLStringForCwd()) {
if (typeof base !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'base', 'string');

this.base = base;
this.isMain = true;

constructor() {
// methods which translate input code or other information
// into es modules
this.translators = translators;
Expand All @@ -71,8 +41,9 @@ class Loader {
this._dynamicInstantiate = undefined;
}

async resolve(specifier, parentURL = this.base) {
if (typeof parentURL !== 'string')
async resolve(specifier, parentURL) {
const isMain = parentURL === undefined;
if (!isMain && typeof parentURL !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'parentURL', 'string');

const { url, format } =
Expand All @@ -93,7 +64,7 @@ class Loader {
return { url, format };
}

async import(specifier, parent = this.base) {
async import(specifier, parent) {
const job = await this.getModuleJob(specifier, parent);
const module = await job.run();
return module.namespace();
Expand All @@ -107,7 +78,7 @@ class Loader {
this._dynamicInstantiate = FunctionBind(dynamicInstantiate, null);
}

async getModuleJob(specifier, parentURL = this.base) {
async getModuleJob(specifier, parentURL) {
const { url, format } = await this.resolve(specifier, parentURL);
let job = this.moduleMap.get(url);
if (job !== undefined)
Expand All @@ -134,24 +105,16 @@ class Loader {
}

let inspectBrk = false;
if (this.isMain) {
if (process._breakFirstLine) {
delete process._breakFirstLine;
inspectBrk = true;
}
this.isMain = false;
if (process._breakFirstLine) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

delete process._breakFirstLine;
inspectBrk = true;
}
job = new ModuleJob(this, url, loaderInstance, inspectBrk);
this.moduleMap.set(url, job);
return job;
}

static registerImportDynamicallyCallback(loader) {
setImportModuleDynamicallyCallback(async (referrer, specifier) => {
return loader.import(specifier, normalizeReferrerURL(referrer));
});
}
}

Object.setPrototypeOf(Loader.prototype, null);

module.exports = Loader;
6 changes: 3 additions & 3 deletions lib/internal/loader/Translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const JsonParse = JSON.parse;
const translators = new SafeMap();
module.exports = translators;

// Stragety for loading a standard JavaScript module
// Strategy for loading a standard JavaScript module
translators.set('esm', async (url) => {
const source = `${await readFileAsync(new URL(url))}`;
debug(`Translating StandardModule ${url}`);
Expand Down Expand Up @@ -62,7 +62,7 @@ translators.set('builtin', async (url) => {
});
});

// Stragety for loading a node native module
// Strategy for loading a node native module
translators.set('addon', async (url) => {
debug(`Translating NativeModule ${url}`);
return createDynamicModule(['default'], url, (reflect) => {
Expand All @@ -74,7 +74,7 @@ translators.set('addon', async (url) => {
});
});

// Stragety for loading a JSON file
// Strategy for loading a JSON file
translators.set('json', async (url) => {
debug(`Translating JSONModule ${url}`);
return createDynamicModule(['default'], url, (reflect) => {
Expand Down
45 changes: 41 additions & 4 deletions lib/internal/process/modules.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,54 @@
'use strict';

const {
setImportModuleDynamicallyCallback,
setInitializeImportMetaObjectCallback
} = internalBinding('module_wrap');

const { getURLFromFilePath } = require('internal/url');
const Loader = require('internal/loader/Loader');
const path = require('path');
const { URL } = require('url');

function normalizeReferrerURL(referrer) {
if (typeof referrer === 'string' && path.isAbsolute(referrer)) {
return getURLFromFilePath(referrer).href;
}
return new URL(referrer).href;
}

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

function setupModules() {
let loaderResolve;
exports.loaderPromise = new Promise((resolve, reject) => {
loaderResolve = resolve;
});

exports.ESMLoader = undefined;

exports.setup = function() {
setInitializeImportMetaObjectCallback(initializeImportMetaObject);
}

module.exports = {
setup: setupModules
let ESMLoader = new Loader();
const loaderPromise = (async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const userLoader = process.binding('config').userLoader;
if (userLoader) {
const hooks = await ESMLoader.import(
userLoader, getURLFromFilePath(`${process.cwd()}/`).href);
ESMLoader = new Loader();
ESMLoader.hook(hooks);
exports.ESMLoader = ESMLoader;
}
return ESMLoader;
})();
loaderResolve(loaderPromise);

setImportModuleDynamicallyCallback(async (referrer, specifier) => {
const loader = await loaderPromise;
return loader.import(specifier, normalizeReferrerURL(referrer));
});

exports.ESMLoader = ESMLoader;
};
29 changes: 8 additions & 21 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
const NativeModule = require('native_module');
const util = require('util');
const { decorateErrorStack } = require('internal/util');
const internalModule = require('internal/module');
const { getURLFromFilePath } = require('internal/url');
const vm = require('vm');
const assert = require('assert').ok;
Expand All @@ -35,6 +34,7 @@ const {
internalModuleReadJSON,
internalModuleStat
} = process.binding('fs');
const internalModule = require('internal/module');
const preserveSymlinks = !!process.binding('config').preserveSymlinks;
const experimentalModules = !!process.binding('config').experimentalModules;

Expand All @@ -43,10 +43,9 @@ const errors = require('internal/errors');
module.exports = Module;

// these are below module.exports for the circular reference
const Loader = require('internal/loader/Loader');
const internalESModule = require('internal/process/modules');
const ModuleJob = require('internal/loader/ModuleJob');
const createDynamicModule = require('internal/loader/CreateDynamicModule');
let ESMLoader;

function stat(filename) {
filename = path.toNamespacedPath(filename);
Expand Down Expand Up @@ -447,7 +446,6 @@ Module._resolveLookupPaths = function(request, parent, newReturn) {
return (newReturn ? parentDir : [id, parentDir]);
};


// Check the cache for the requested file.
// 1. If a module already exists in the cache: return its exports object.
// 2. If the module is native: call `NativeModule.require()` with the
Expand All @@ -460,22 +458,10 @@ Module._load = function(request, parent, isMain) {
debug('Module._load REQUEST %s parent: %s', request, parent.id);
}

if (isMain && experimentalModules) {
(async () => {
// loader setup
if (!ESMLoader) {
ESMLoader = new Loader();
const userLoader = process.binding('config').userLoader;
if (userLoader) {
ESMLoader.isMain = false;
const hooks = await ESMLoader.import(userLoader);
ESMLoader = new Loader();
ESMLoader.hook(hooks);
}
}
Loader.registerImportDynamicallyCallback(ESMLoader);
await ESMLoader.import(getURLFromFilePath(request).pathname);
})()
if (experimentalModules && isMain) {
internalESModule.loaderPromise.then((loader) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related reading??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For adding the behavior for error messages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return loader.import(getURLFromFilePath(request).pathname);
})
.catch((e) => {
decorateErrorStack(e);
console.error(e);
Expand Down Expand Up @@ -578,7 +564,8 @@ Module.prototype.load = function(filename) {
Module._extensions[extension](this, filename);
this.loaded = true;

if (ESMLoader) {
if (experimentalModules) {
const ESMLoader = internalESModule.ESMLoader;
const url = getURLFromFilePath(filename);
const urlString = `${url}`;
const exports = this.exports;
Expand Down
8 changes: 8 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ class performance_state;

namespace loader {
class ModuleWrap;
struct PackageConfig {
const bool exists;
const bool is_valid;
const bool has_main;
const std::string main;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For legibility reasons, it might be better to use enums instead of bools:

struct PackageConfig {
  const enum { Exists, DoesNotExist } exists;
  const enum { Valid, Invalid } is_valid;
  const enum { HasMain, NoMain } has_main;
  const std::string main;
};
// init with PackageConfig { PackageConfig::Exists, PackageConfig::Valid, PackageConfig::NoMain, "" }

Adds a little extra type safety because the enums are distinct types, i.e., you can't assign Invalid to .exists.

There's an alternative version that looks like this:

struct Exists {
  enum Bool { Yes, No };
};

struct IsValid {
  enum Bool { Yes, No };
};

struct HasMain {
  enum Bool { Yes, No };
};

struct PackageConfig {
  const Exists::Bool exists;
  const IsValid::Bool is_valid;
  const HasMain::Bool has_main;
  const std::string main;
};
// init with PackageConfig { Exists::Yes, IsValid::Yes, HasMain::No, "" }

I personally prefer the second one but it's slightly more laborious to write.

}

// Pick an index that's hopefully out of the way when we're embedded inside
Expand Down Expand Up @@ -608,6 +614,8 @@ class Environment {

std::unordered_multimap<int, loader::ModuleWrap*> module_map;

std::unordered_map<std::string, loader::PackageConfig> package_json_cache;

inline double* heap_statistics_buffer() const;
inline void set_heap_statistics_buffer(double* pointer);

Expand Down
Loading