GeezerVison is showing github.com — Revert "esm: convert `resolve` hook to synchronous" · nodejs/node@3c04034
File treeExpand file treeCollapse file tree Open diff view settings fixtures/es-module-loaders
Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: doc/api/esm.md +8-21Lines changed: 8 additions & 21 deletions - Display the source diff
- Display the rich diff
| Original file line number | Diff line number | Diff line change |
|---|
@@ -324,9 +324,6 @@ added: | 324 | 324 | - v13.9.0 | 325 | 325 | - v12.16.2 | 326 | 326 | changes: | 327 | | - - version: REPLACEME | 328 | | - pr-url: https://github.com/nodejs/node/pull/43363 | 329 | | - description: Convert from asynchronous to synchronous. | 330 | 327 | - version: | 331 | 328 | - v16.2.0 | 332 | 329 | - v14.18.0 | @@ -342,19 +339,15 @@ command flag enabled. | 342 | 339 | * `specifier` {string} The module specifier to resolve relative to `parent`. | 343 | 340 | * `parent` {string|URL} The absolute parent module URL to resolve from. If none | 344 | 341 | is specified, the value of `import.meta.url` is used as the default. | 345 | | -* Returns: {string} | | 342 | +* Returns: {Promise} | 346 | 343 |
| 347 | 344 | Provides a module-relative resolution function scoped to each module, returning | 348 | | -the URL string. In alignment with browser behavior, this now returns | 349 | | -synchronously. | 350 | | -
| 351 | | -> **Caveat** This can result in synchronous file-system operations, which | 352 | | -> can impact performance similarly to `require.resolve`. | | 345 | +the URL string. | 353 | 346 |
| 354 | 347 | <!-- eslint-skip --> | 355 | 348 |
| 356 | 349 | ```js | 357 | | -const dependencyAsset = import.meta.resolve('component-lib/asset.css'); | | 350 | +const dependencyAsset = await import.meta.resolve('component-lib/asset.css'); | 358 | 351 | ``` | 359 | 352 |
| 360 | 353 | `import.meta.resolve` also accepts a second argument which is the parent module | @@ -363,11 +356,11 @@ from which to resolve from: | 363 | 356 | <!-- eslint-skip --> | 364 | 357 |
| 365 | 358 | ```js | 366 | | -import.meta.resolve('./dep', import.meta.url); | | 359 | +await import.meta.resolve('./dep', import.meta.url); | 367 | 360 | ``` | 368 | 361 |
| 369 | | -This function is synchronous because the ES module resolver in Node.js is | 370 | | -synchronous. | | 362 | +This function is asynchronous because the ES module resolver in Node.js is | | 363 | +allowed to be asynchronous. | 371 | 364 |
| 372 | 365 | ## Interoperability with CommonJS | 373 | 366 |
| @@ -738,9 +731,6 @@ prevent unintentional breaks in the chain. | 738 | 731 |
| 739 | 732 | <!-- YAML | 740 | 733 | changes: | 741 | | - - version: REPLACEME | 742 | | - pr-url: https://github.com/nodejs/node/pull/43363 | 743 | | - description: Convert hook from asynchronous to synchronous. | 744 | 734 | - version: REPLACEME | 745 | 735 | pr-url: https://github.com/nodejs/node/pull/42623 | 746 | 736 | description: Add support for chaining resolve hooks. Each hook must either | @@ -774,9 +764,6 @@ changes: | 774 | 764 | terminate the chain of `resolve` hooks. **Default:** `false` | 775 | 765 | * `url` {string} The absolute URL to which this input resolves | 776 | 766 |
| 777 | | -> **Caveat** A resolve hook can contain synchronous file-system operations | 778 | | -> (as `defaultResolveHook()` does), which can impact performance. | 779 | | -
| 780 | 767 | The `resolve` hook chain is responsible for resolving file URL for a given | 781 | 768 | module specifier and parent URL, and optionally its format (such as `'module'`) | 782 | 769 | as a hint to the `load` hook. If a format is specified, the `load` hook is | @@ -803,7 +790,7 @@ Node.js module specifier resolution behavior_ when calling `defaultResolve`, the | 803 | 790 | `context.conditions` array originally passed into the `resolve` hook. | 804 | 791 |
| 805 | 792 | ```js | 806 | | -export function resolve(specifier, context, nextResolve) { | | 793 | +export async function resolve(specifier, context, nextResolve) { | 807 | 794 | const { parentURL = null } = context; | 808 | 795 |
| 809 | 796 | if (Math.random() > 0.5) { // Some condition. | @@ -1102,7 +1089,7 @@ const baseURL = pathToFileURL(`${cwd()}/`).href; | 1102 | 1089 | // CoffeeScript files end in .coffee, .litcoffee, or .coffee.md. | 1103 | 1090 | const extensionsRegex = /\.coffee$|\.litcoffee$|\.coffee\.md$/; | 1104 | 1091 |
| 1105 | | -export function resolve(specifier, context, nextResolve) { | | 1092 | +export async function resolve(specifier, context, nextResolve) { | 1106 | 1093 | if (extensionsRegex.test(specifier)) { | 1107 | 1094 | const { parentURL = baseURL } = context; | 1108 | 1095 |
| |
Collapse file Copy file name to clipboardExpand all lines: lib/internal/modules/esm/initialize_import_meta.js +12-14Lines changed: 12 additions & 14 deletions | Original file line number | Diff line number | Diff line change |
|---|
| 3 | 3 | const { getOptionValue } = require('internal/options'); | 4 | 4 | const experimentalImportMetaResolve = | 5 | 5 | getOptionValue('--experimental-import-meta-resolve'); | | 6 | +const { | | 7 | + PromisePrototypeThen, | | 8 | + PromiseReject, | | 9 | +} = primordials; | 6 | 10 | const asyncESM = require('internal/process/esm_loader'); | 7 | 11 |
| 8 | 12 | function createImportMetaResolve(defaultParentUrl) { | 9 | | - return function resolve(specifier, parentUrl = defaultParentUrl) { | 10 | | - let url; | 11 | | - | 12 | | - try { | 13 | | - ({ url } = asyncESM.esmLoader.resolve(specifier, parentUrl)); | 14 | | - } catch (error) { | 15 | | - if (error.code === 'ERR_UNSUPPORTED_DIR_IMPORT') { | 16 | | - ({ url } = error); | 17 | | - } else { | 18 | | - throw error; | 19 | | - } | 20 | | - } | 21 | | - | 22 | | - return url; | | 13 | + return async function resolve(specifier, parentUrl = defaultParentUrl) { | | 14 | + return PromisePrototypeThen( | | 15 | + asyncESM.esmLoader.resolve(specifier, parentUrl), | | 16 | + ({ url }) => url, | | 17 | + (error) => ( | | 18 | + error.code === 'ERR_UNSUPPORTED_DIR_IMPORT' ? | | 19 | + error.url : PromiseReject(error)) | | 20 | + ); | 23 | 21 | }; | 24 | 22 | } | 25 | 23 |
| |
Collapse file Copy file name to clipboardExpand all lines: lib/internal/modules/esm/loader.js +31-64Lines changed: 31 additions & 64 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -15,8 +15,6 @@ const { | 15 | 15 | ObjectDefineProperty, | 16 | 16 | ObjectSetPrototypeOf, | 17 | 17 | PromiseAll, | 18 | | - PromiseResolve, | 19 | | - PromisePrototypeThen, | 20 | 18 | ReflectApply, | 21 | 19 | RegExpPrototypeExec, | 22 | 20 | SafeArrayIterator, | @@ -111,14 +109,12 @@ let emittedSpecifierResolutionWarning = false; | 111 | 109 | * position in the hook chain. | 112 | 110 | * @param {string} meta.hookName The kind of hook the chain is (ex 'resolve') | 113 | 111 | * @param {boolean} meta.shortCircuited Whether a hook signaled a short-circuit. | 114 | | - * @param {object} validators A wrapper function | | 112 | + * @param {(hookErrIdentifier, hookArgs) => void} validate A wrapper function | 115 | 113 | * containing all validation of a custom loader hook's intermediary output. Any | 116 | 114 | * validation within MUST throw. | 117 | | - * @param {(hookErrIdentifier, hookArgs) => void} validators.validateArgs | 118 | | - * @param {(hookErrIdentifier, output) => void} validators.validateOutput | 119 | 115 | * @returns {function next<HookName>(...hookArgs)} The next hook in the chain. | 120 | 116 | */ | 121 | | -function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { | | 117 | +function nextHookFactory(chain, meta, validate) { | 122 | 118 | // First, prepare the current | 123 | 119 | const { hookName } = meta; | 124 | 120 | const { | @@ -141,7 +137,7 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { | 141 | 137 | // factory generates the next link in the chain. | 142 | 138 | meta.hookIndex--; | 143 | 139 |
| 144 | | - nextNextHook = nextHookFactory(chain, meta, { validateArgs, validateOutput }); | | 140 | + nextNextHook = nextHookFactory(chain, meta, validate); | 145 | 141 | } else { | 146 | 142 | // eslint-disable-next-line func-name-matching | 147 | 143 | nextNextHook = function chainAdvancedTooFar() { | @@ -152,36 +148,21 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { | 152 | 148 | } | 153 | 149 |
| 154 | 150 | return ObjectDefineProperty( | 155 | | - (...args) => { | | 151 | + async (...args) => { | 156 | 152 | // Update only when hook is invoked to avoid fingering the wrong filePath | 157 | 153 | meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`; | 158 | 154 |
| 159 | | - validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args); | 160 | | - | 161 | | - const outputErrIdentifier = `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`; | | 155 | + validate(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args); | 162 | 156 |
| 163 | 157 | // Set when next<HookName> is actually called, not just generated. | 164 | 158 | if (generatedHookIndex === 0) { meta.chainFinished = true; } | 165 | 159 |
| 166 | 160 | ArrayPrototypePush(args, nextNextHook); | 167 | | - const output = ReflectApply(hook, undefined, args); | 168 | | - | 169 | | - validateOutput(outputErrIdentifier, output); | | 161 | + const output = await ReflectApply(hook, undefined, args); | 170 | 162 |
| 171 | | - function checkShortCircuited(output) { | 172 | | - if (output?.shortCircuit === true) { meta.shortCircuited = true; } | 173 | | - | 174 | | - return output; | 175 | | - } | 176 | | - | 177 | | - if (meta.isChainAsync) { | 178 | | - return PromisePrototypeThen( | 179 | | - PromiseResolve(output), | 180 | | - checkShortCircuited, | 181 | | - ); | 182 | | - } | | 163 | + if (output?.shortCircuit === true) { meta.shortCircuited = true; } | | 164 | + return output; | 183 | 165 |
| 184 | | - return checkShortCircuited(output); | 185 | 166 | }, | 186 | 167 | 'name', | 187 | 168 | { __proto__: null, value: nextHookName }, | @@ -440,11 +421,8 @@ class ESMLoader { | 440 | 421 | ); | 441 | 422 | } | 442 | 423 |
| 443 | | - const { format, url } = this.resolve( | 444 | | - specifier, | 445 | | - parentURL, | 446 | | - importAssertionsForResolve, | 447 | | - ); | | 424 | + const { format, url } = | | 425 | + await this.resolve(specifier, parentURL, importAssertionsForResolve); | 448 | 426 |
| 449 | 427 | let job = this.moduleMap.get(url, importAssertions.type); | 450 | 428 |
| @@ -579,11 +557,10 @@ class ESMLoader { | 579 | 557 | hookErrIdentifier: '', | 580 | 558 | hookIndex: chain.length - 1, | 581 | 559 | hookName: 'load', | 582 | | - isChainAsync: true, | 583 | 560 | shortCircuited: false, | 584 | 561 | }; | 585 | 562 |
| 586 | | - const validateArgs = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => { | | 563 | + const validate = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => { | 587 | 564 | if (typeof nextUrl !== 'string') { | 588 | 565 | // non-strings can be coerced to a url string | 589 | 566 | // validateString() throws a less-specific error | @@ -609,22 +586,19 @@ class ESMLoader { | 609 | 586 |
| 610 | 587 | validateObject(ctx, `${hookErrIdentifier} context`); | 611 | 588 | }; | 612 | | - const validateOutput = (hookErrIdentifier, output) => { | 613 | | - if (typeof output !== 'object') { // [2] | 614 | | - throw new ERR_INVALID_RETURN_VALUE( | 615 | | - 'an object', | 616 | | - hookErrIdentifier, | 617 | | - output, | 618 | | - ); | 619 | | - } | 620 | | - }; | 621 | 589 |
| 622 | | - const nextLoad = nextHookFactory(chain, meta, { validateArgs, validateOutput }); | | 590 | + const nextLoad = nextHookFactory(chain, meta, validate); | 623 | 591 |
| 624 | 592 | const loaded = await nextLoad(url, context); | 625 | 593 | const { hookErrIdentifier } = meta; // Retrieve the value after all settled | 626 | 594 |
| 627 | | - validateOutput(hookErrIdentifier, loaded); | | 595 | + if (typeof loaded !== 'object') { // [2] | | 596 | + throw new ERR_INVALID_RETURN_VALUE( | | 597 | + 'an object', | | 598 | + hookErrIdentifier, | | 599 | + loaded, | | 600 | + ); | | 601 | + } | 628 | 602 |
| 629 | 603 | if (loaded?.shortCircuit === true) { meta.shortCircuited = true; } | 630 | 604 |
| @@ -804,7 +778,7 @@ class ESMLoader { | 804 | 778 | * statement or expression. | 805 | 779 | * @returns {{ format: string, url: URL['href'] }} | 806 | 780 | */ | 807 | | - resolve( | | 781 | + async resolve( | 808 | 782 | originalSpecifier, | 809 | 783 | parentURL, | 810 | 784 | importAssertions = ObjectCreate(null) | @@ -828,43 +802,36 @@ class ESMLoader { | 828 | 802 | hookErrIdentifier: '', | 829 | 803 | hookIndex: chain.length - 1, | 830 | 804 | hookName: 'resolve', | 831 | | - isChainAsync: false, | 832 | 805 | shortCircuited: false, | 833 | 806 | }; | | 807 | + | 834 | 808 | const context = { | 835 | 809 | conditions: DEFAULT_CONDITIONS, | 836 | 810 | importAssertions, | 837 | 811 | parentURL, | 838 | 812 | }; | | 813 | + const validate = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => { | 839 | 814 |
| 840 | | - const validateArgs = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => { | 841 | 815 | validateString( | 842 | 816 | suppliedSpecifier, | 843 | 817 | `${hookErrIdentifier} specifier`, | 844 | 818 | ); // non-strings can be coerced to a url string | 845 | 819 |
| 846 | 820 | validateObject(ctx, `${hookErrIdentifier} context`); | 847 | 821 | }; | 848 | | - const validateOutput = (hookErrIdentifier, output) => { | 849 | | - if ( | 850 | | - typeof output !== 'object' || // [2] | 851 | | - output === null || | 852 | | - (output.url == null && typeof output.then === 'function') | 853 | | - ) { | 854 | | - throw new ERR_INVALID_RETURN_VALUE( | 855 | | - 'an object', | 856 | | - hookErrIdentifier, | 857 | | - output, | 858 | | - ); | 859 | | - } | 860 | | - }; | 861 | 822 |
| 862 | | - const nextResolve = nextHookFactory(chain, meta, { validateArgs, validateOutput }); | | 823 | + const nextResolve = nextHookFactory(chain, meta, validate); | 863 | 824 |
| 864 | | - const resolution = nextResolve(originalSpecifier, context); | | 825 | + const resolution = await nextResolve(originalSpecifier, context); | 865 | 826 | const { hookErrIdentifier } = meta; // Retrieve the value after all settled | 866 | 827 |
| 867 | | - validateOutput(hookErrIdentifier, resolution); | | 828 | + if (typeof resolution !== 'object') { // [2] | | 829 | + throw new ERR_INVALID_RETURN_VALUE( | | 830 | + 'an object', | | 831 | + hookErrIdentifier, | | 832 | + resolution, | | 833 | + ); | | 834 | + } | 868 | 835 |
| 869 | 836 | if (resolution?.shortCircuit === true) { meta.shortCircuited = true; } | 870 | 837 |
| |
Collapse file Copy file name to clipboardExpand all lines: lib/internal/modules/esm/resolve.js +3-3Lines changed: 3 additions & 3 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -1081,7 +1081,7 @@ function throwIfUnsupportedURLScheme(parsed, experimentalNetworkImports) { | 1081 | 1081 | } | 1082 | 1082 | } | 1083 | 1083 |
| 1084 | | -function defaultResolve(specifier, context = {}) { | | 1084 | +async function defaultResolve(specifier, context = {}) { | 1085 | 1085 | let { parentURL, conditions } = context; | 1086 | 1086 | if (parentURL && policy?.manifest) { | 1087 | 1087 | const redirects = policy.manifest.getDependencyMapper(parentURL); | @@ -1227,11 +1227,11 @@ const { | 1227 | 1227 |
| 1228 | 1228 | if (policy) { | 1229 | 1229 | const $defaultResolve = defaultResolve; | 1230 | | - module.exports.defaultResolve = function defaultResolve( | | 1230 | + module.exports.defaultResolve = async function defaultResolve( | 1231 | 1231 | specifier, | 1232 | 1232 | context | 1233 | 1233 | ) { | 1234 | | - const ret = $defaultResolve(specifier, context); | | 1234 | + const ret = await $defaultResolve(specifier, context, $defaultResolve); | 1235 | 1235 | // This is a preflight check to avoid data exfiltration by query params etc. | 1236 | 1236 | policy.manifest.mightAllow(ret.url, () => | 1237 | 1237 | new ERR_MANIFEST_DEPENDENCY_MISSING( | |
|