-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
Changes from 1 commit
f14420e
adfe1ac
ebdb3cb
5855e5c
05c9941
2d6d4e6
a06db2d
081aab4
ed9df8a
8b4d85f
4468e2c
b4dc802
9f7aabd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,16 +117,19 @@ 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) { | ||
return { | ||
url: new URL(specifier, parentModuleURL).href, | ||
url: new URL(specifier, parentModuleURL || baseURL).href, | ||
format: 'esm' | ||
}; | ||
} | ||
``` | ||
|
||
The parentURL is provided as `undefined` when performing main NodeJS load itself. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
|
||
The default NodeJS ES module resolution function is provided as a third | ||
argument to the resolver for easy compatibility workflows. | ||
|
||
|
@@ -155,7 +158,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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
@@ -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(); | ||
|
||
|
@@ -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')) | ||
|
@@ -66,17 +67,22 @@ 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 }; | ||
|
||
const format = extensionFormatMap[ext] || parentURL === undefined && 'cjs'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is confusing IMO, I'd write it as either an if/else or at least use parenthesis. |
||
if (!format) | ||
throw new errors.Error('ERR_UNKNOWN_FILE_EXTENSION', url.pathname); | ||
|
||
return { url: `${url}`, format }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just wondering, why is this wrapping of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
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; | ||
|
@@ -71,8 +41,8 @@ class Loader { | |
this._dynamicInstantiate = undefined; | ||
} | ||
|
||
async resolve(specifier, parentURL = this.base) { | ||
if (typeof parentURL !== 'string') | ||
async resolve(specifier, parentURL) { | ||
if (parentURL !== undefined && typeof parentURL !== 'string') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I like the new logic (of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Loaders exist in their own context, without even a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, all I'm saying is that I'd likely put a function called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to be a bit careful about over abstracting here if we ever want to support a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, we could assign to |
||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'parentURL', 'string'); | ||
|
||
const { url, format } = | ||
|
@@ -93,7 +63,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(); | ||
|
@@ -107,7 +77,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) | ||
|
@@ -134,24 +104,16 @@ class Loader { | |
} | ||
|
||
let inspectBrk = false; | ||
if (this.isMain) { | ||
if (process._breakFirstLine) { | ||
delete process._breakFirstLine; | ||
inspectBrk = true; | ||
} | ||
this.isMain = false; | ||
if (process._breakFirstLine) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading this again, can you explain why removing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we clear |
||
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,52 @@ | ||
'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() { | ||
setInitializeImportMetaObjectCallback(initializeImportMetaObject); | ||
|
||
let ESMLoader = new Loader(); | ||
const loaderPromise = (async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
})(); | ||
loaderPromise.catch(() => {}); | ||
|
||
setImportModuleDynamicallyCallback(async (referrer, specifier) => { | ||
const loader = await loaderPromise; | ||
return loader.import(specifier, normalizeReferrerURL(referrer)); | ||
}); | ||
|
||
exports.loaderPromise = loaderPromise; | ||
exports.ESMLoader = ESMLoader; | ||
} | ||
|
||
module.exports = { | ||
setup: setupModules | ||
}; | ||
exports.setup = setupModules; | ||
exports.ESMLoader = undefined; | ||
exports.loaderPromise = undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we want to start this as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure I'll switch around some of the async logic. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
||
|
@@ -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); | ||
|
@@ -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 | ||
|
@@ -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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a fan of our error handling logic here of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bmeck then we should either fix the unhandled rejection error messages or If you can point me towards related reading I promise to read it :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. related reading?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For adding the behavior for error messages There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.