Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
aduh95 authored Sep 15, 2023
1 parent 6557d26 commit 3be8276
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 29 deletions.
72 changes: 54 additions & 18 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,9 @@ ObjectDefineProperty(Module, '_stat', {

function updateChildren(parent, child, scan) {
const children = parent?.children;
if (children && !(scan && ArrayPrototypeIncludes(children, child))) { ArrayPrototypePush(children, child); }
if (children && !(scan && ArrayPrototypeIncludes(children, child))) {
ArrayPrototypePush(children, child);
}
}

function reportModuleToWatchMode(filename) {
Expand Down Expand Up @@ -385,7 +387,9 @@ function readPackageScope(checkPath) {
if (enabledPermission && !permission.has('fs.read', checkPath + sep)) {
return false;
}
if (StringPrototypeEndsWith(checkPath, sep + 'node_modules')) { return false; }
if (StringPrototypeEndsWith(checkPath, sep + 'node_modules')) {
return false;
}
const pjson = _readPackage(checkPath + sep);
if (pjson.exists) {
return {
Expand Down Expand Up @@ -523,7 +527,9 @@ function trySelf(parentPath, request) {
pathToFileURL(pkgPath + '/package.json'), expansion, pkg,
pathToFileURL(parentPath), getCjsConditions()), parentPath, pkgPath);
} catch (e) {
if (e.code === 'ERR_MODULE_NOT_FOUND') { throw createEsmNotFoundErr(request, pkgPath + '/package.json'); }
if (e.code === 'ERR_MODULE_NOT_FOUND') {
throw createEsmNotFoundErr(request, pkgPath + '/package.json');
}
throw e;
}
}
Expand All @@ -546,7 +552,9 @@ function resolveExports(nmPath, request) {
pathToFileURL(pkgPath + '/package.json'), '.' + expansion, pkg, null,
getCjsConditions()), null, pkgPath);
} catch (e) {
if (e.code === 'ERR_MODULE_NOT_FOUND') { throw createEsmNotFoundErr(request, pkgPath + '/package.json'); }
if (e.code === 'ERR_MODULE_NOT_FOUND') {
throw createEsmNotFoundErr(request, pkgPath + '/package.json');
}
throw e;
}
}
Expand All @@ -568,7 +576,9 @@ Module._findPath = function(request, paths, isMain) {

const cacheKey = request + '\x00' + ArrayPrototypeJoin(paths, '\x00');
const entry = Module._pathCache[cacheKey];
if (entry) { return entry; }
if (entry) {
return entry;
}

let exts;
const trailingSlash = request.length > 0 &&
Expand Down Expand Up @@ -615,7 +625,9 @@ Module._findPath = function(request, paths, isMain) {

if (!absoluteRequest) {
const exportsResolved = resolveExports(curPath, request);
if (exportsResolved) { return exportsResolved; }
if (exportsResolved) {
return exportsResolved;
}
}

const basePath = path.resolve(curPath, request);
Expand Down Expand Up @@ -647,14 +659,18 @@ Module._findPath = function(request, paths, isMain) {

if (!filename) {
// Try it with each of the extensions
if (exts === undefined) { exts = ObjectKeys(Module._extensions); }
if (exts === undefined) {
exts = ObjectKeys(Module._extensions);
}
filename = tryExtensions(basePath, exts, isMain);
}
}

if (!filename && rc === 1) { // Directory.
// try it with each of the extensions at "index"
if (exts === undefined) { exts = ObjectKeys(Module._extensions); }
if (exts === undefined) {
exts = ObjectKeys(Module._extensions);
}
filename = tryPackage(basePath, exts, isMain, request);
}

Expand Down Expand Up @@ -690,7 +706,9 @@ if (isWindows) {
// path.resolve will make sure from.length >=3 in Windows.
if (StringPrototypeCharCodeAt(from, from.length - 1) ===
CHAR_BACKWARD_SLASH &&
StringPrototypeCharCodeAt(from, from.length - 2) === CHAR_COLON) { return [from + 'node_modules']; }
StringPrototypeCharCodeAt(from, from.length - 2) === CHAR_COLON) {
return [from + 'node_modules'];

Check failure on line 710 in lib/internal/modules/cjs/loader.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Expected indentation of 6 spaces but found 10
}

Check failure on line 711 in lib/internal/modules/cjs/loader.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Expected indentation of 4 spaces but found 8

const paths = [];
for (let i = from.length - 1, p = 0, last = from.length; i >= 0; --i) {
Expand Down Expand Up @@ -729,7 +747,9 @@ if (isWindows) {
from = path.resolve(from);
// Return early not only to avoid unnecessary work, but to *avoid* returning
// an array of two items for a root: [ '//node_modules', '/node_modules' ]
if (from === '/') { return ['/node_modules']; }
if (from === '/') {
return ['/node_modules'];
}

// note: this approach *only* works when the path is guaranteed
// to be absolute. Doing a fully-edge-case-correct path.split
Expand Down Expand Up @@ -872,7 +892,9 @@ Module._load = function(request, parent, isMain) {
const cachedModule = Module._cache[filename];
if (cachedModule !== undefined) {
updateChildren(parent, cachedModule, true);
if (!cachedModule.loaded) { return getExportsForCircularRequire(cachedModule); }
if (!cachedModule.loaded) {
return getExportsForCircularRequire(cachedModule);
}
return cachedModule.exports;
}
delete relativeResolveCache[relResolveCacheIdentifier];
Expand All @@ -897,7 +919,9 @@ Module._load = function(request, parent, isMain) {
updateChildren(parent, cachedModule, true);
if (!cachedModule.loaded) {
const parseCachedModule = cjsParseCache.get(cachedModule);
if (!parseCachedModule || parseCachedModule.loaded) { return getExportsForCircularRequire(cachedModule); }
if (!parseCachedModule || parseCachedModule.loaded) {
return getExportsForCircularRequire(cachedModule);
}
parseCachedModule.loaded = true;
} else {
return cachedModule.exports;
Expand Down Expand Up @@ -980,7 +1004,9 @@ Module._resolveFilename = function(request, parent, isMain, options) {
const lookupPaths = Module._resolveLookupPaths(request, fakeParent);

for (let j = 0; j < lookupPaths.length; j++) {
if (!ArrayPrototypeIncludes(paths, lookupPaths[j])) { ArrayPrototypePush(paths, lookupPaths[j]); }
if (!ArrayPrototypeIncludes(paths, lookupPaths[j])) {
ArrayPrototypePush(paths, lookupPaths[j]);
}
}
}
}
Expand All @@ -1004,7 +1030,9 @@ Module._resolveFilename = function(request, parent, isMain, options) {
getCjsConditions()), parentPath,
pkg.path);
} catch (e) {
if (e.code === 'ERR_MODULE_NOT_FOUND') { throw createEsmNotFoundErr(request); }
if (e.code === 'ERR_MODULE_NOT_FOUND') {
throw createEsmNotFoundErr(request);
}
throw e;
}
}
Expand Down Expand Up @@ -1049,7 +1077,9 @@ function finalizeEsmResolution(resolved, parentPath, pkgPath) {
}
const filename = fileURLToPath(resolved);
const actual = tryFile(filename);
if (actual) { return actual; }
if (actual) {
return actual;
}
const err = createEsmNotFoundErr(filename,
path.resolve(pkgPath, 'package.json'));
throw err;
Expand All @@ -1059,7 +1089,9 @@ function createEsmNotFoundErr(request, path) {
// eslint-disable-next-line no-restricted-syntax
const err = new Error(`Cannot find module '${request}'`);
err.code = 'MODULE_NOT_FOUND';
if (path) { err.path = path; }
if (path) {
err.path = path;
}
// TODO(BridgeAR): Add the requireStack as well.
return err;
}
Expand Down Expand Up @@ -1087,7 +1119,9 @@ Module.prototype.load = function(filename) {
// Preemptively cache
if ((module?.module === undefined ||
module.module.getStatus() < kEvaluated) &&
!cascadedLoader.cjsCache.has(this)) { cascadedLoader.cjsCache.set(this, exports); }
!cascadedLoader.cjsCache.has(this)) {
cascadedLoader.cjsCache.set(this, exports);
}
};

// Loads a module at the given file path. Returns that module's
Expand Down Expand Up @@ -1404,7 +1438,9 @@ Module._preloadModules = function(requests) {
throw e;
}
}
for (let n = 0; n < requests.length; n++) { internalRequire(parent, requests[n]); }
for (let n = 0; n < requests.length; n++) {
internalRequire(parent, requests[n]);
}
isPreloading = false;
};

Expand Down
8 changes: 6 additions & 2 deletions lib/internal/modules/esm/create_dynamic_module.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,17 @@ import.meta.done();
onReady: (cb) => { readyfns.add(cb); },
};

if (imports.length) { reflect.imports = { __proto__: null }; }
if (imports.length) {
reflect.imports = { __proto__: null };
}
const { registerModule } = require('internal/modules/esm/utils');
registerModule(m, {
__proto__: null,
initializeImportMeta: (meta, wrap) => {
meta.exports = reflect.exports;
if (reflect.imports) { meta.imports = reflect.imports; }
if (reflect.imports) {
meta.imports = reflect.imports;
}
meta.done = () => {
evaluate(reflect);
reflect.onReady = (cb) => cb(reflect);
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@ class ModuleJob {
return job.modulePromise;
});

if (promises !== undefined) { await SafePromiseAllReturnVoid(promises); }
if (promises !== undefined) {
await SafePromiseAllReturnVoid(promises);
}

return SafePromiseAllReturnArrayLike(dependencyJobs);
};
Expand Down
12 changes: 9 additions & 3 deletions lib/internal/modules/esm/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,9 @@ function resolvePackageTarget(packageJSONUrl, target, subpath, packageSubpath,
}
return resolveResult;
}
if (lastException === undefined || lastException === null) { return lastException; }
if (lastException === undefined || lastException === null) {
return lastException;
}
throw lastException;
} else if (typeof target === 'object' && target !== null) {
const keys = ObjectGetOwnPropertyNames(target);
Expand Down Expand Up @@ -516,7 +518,9 @@ function isConditionalExportsMainSugar(exports, packageJSONUrl, base) {
function packageExportsResolve(
packageJSONUrl, packageSubpath, packageConfig, base, conditions) {
let exports = packageConfig.exports;
if (isConditionalExportsMainSugar(exports, packageJSONUrl, base)) { exports = { '.': exports }; }
if (isConditionalExportsMainSugar(exports, packageJSONUrl, base)) {
exports = { '.': exports };
}

if (ObjectPrototypeHasOwnProperty(exports, packageSubpath) &&
!StringPrototypeIncludes(packageSubpath, '*') &&
Expand Down Expand Up @@ -701,7 +705,9 @@ function parsePackageName(specifier, base) {

// Package name cannot have leading . and cannot have percent-encoding or
// \\ separators.
if (RegExpPrototypeExec(invalidPackageNameRegEx, packageName) !== null) { validPackageName = false; }
if (RegExpPrototypeExec(invalidPackageNameRegEx, packageName) !== null) {
validPackageName = false;
}

if (!validPackageName) {
throw new ERR_INVALID_MODULE_SPECIFIER(
Expand Down
16 changes: 12 additions & 4 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,9 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
}
for (const exportName of exportNames) {
if (!ObjectPrototypeHasOwnProperty(exports, exportName) ||
exportName === 'default') { continue; }
exportName === 'default') {
continue;
}
// We might trigger a getter -> dont fail.
let value;
try {
Expand Down Expand Up @@ -302,7 +304,9 @@ function cjsPreparseModuleExports(filename, source) {
let module = CJSModule._cache[filename];
if (module) {
const cached = cjsParseCache.get(module);
if (cached) { return { module, exportNames: cached.exportNames }; }
if (cached) {
return { module, exportNames: cached.exportNames };
}
}
const loaded = Boolean(module);
if (!loaded) {
Expand Down Expand Up @@ -353,7 +357,9 @@ function cjsPreparseModuleExports(filename, source) {
// (and fallback to reading the FS only if the source is nullish).
const source = readFileSync(resolved, 'utf-8');
const { exportNames: reexportNames } = cjsPreparseModuleExports(resolved, source);
for (const name of reexportNames) { exportNames.add(name); }
for (const name of reexportNames) {
exportNames.add(name);
}
}
}
}
Expand Down Expand Up @@ -462,6 +468,8 @@ translators.set('wasm', async function(url, source) {
'internal/modules/esm/create_dynamic_module');
return createDynamicModule(imports, exports, url, (reflect) => {
const { exports } = new WebAssembly.Instance(compiled, reflect.imports);
for (const expt of ObjectKeys(exports)) { reflect.exports[expt].set(exports[expt]); }
for (const expt of ObjectKeys(exports)) {
reflect.exports[expt].set(exports[expt]);
}
}).module;
});
4 changes: 3 additions & 1 deletion lib/internal/modules/run_main.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ function resolveMainPath(main) {
if (!mainPath) { return; }

const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
if (!preserveSymlinksMain) { mainPath = toRealPath(mainPath); }
if (!preserveSymlinksMain) {
mainPath = toRealPath(mainPath);
}

return mainPath;
}
Expand Down

0 comments on commit 3be8276

Please sign in to comment.