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 @@ -608,6 +608,14 @@ class Environment {

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

struct PackageConfig {
bool exists;
bool is_valid;
bool has_main;
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.

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

Copy link
Member

Choose a reason for hiding this comment

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

Would still be nice if these were const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

Expand Down
Loading