Skip to content

Commit

Permalink
loader: fix package resolution for edge case
Browse files Browse the repository at this point in the history 
this commit solves a regression introduced with PR-40980.
if a resolve call results in a script with .mjs extension the
is automatically set to . This avoids the case where an additional
 in the same directory as the .mjs file would declare the
 to commonjs
  • Loading branch information
@dygabo
dygabo committed Dec 17, 2021
1 parent a182a21 commit a149086
Show file tree
Hide file tree
Showing 2 changed files with 169 additions and 78 deletions.
7 changes: 6 additions & 1 deletion lib/internal/modules/esm/resolve.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,12 @@ function resolvePackageTargetString(
const composeResult = (resolved) => {
let format;
try {
format = getPackageType(resolved);
// extension has higher priority than package.json type descriptor
if (StringPrototypeEndsWith(resolved.href, '.mjs')) {
format = 'module';
} else {
format = getPackageType(resolved);
}
} catch (err) {
if (err.code === 'ERR_INVALID_FILE_URL_PATH') {
const invalidModuleErr = new ERR_INVALID_MODULE_SPECIFIER(
Expand Down
240 changes: 163 additions & 77 deletions test/es-module/test-esm-resolve-type.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,86 +98,172 @@ try {
}
};

// Create a dummy dual package
//
/**
* this creates following directory structure:
*
* ./node_modules:
* |-> my-dual-package
* |-> es
* |-> index.js
* |-> package.json [2]
* |-> lib
* |-> index.js
* |->package.json [1]
*
* [1] - main package.json of the package
* - it contains:
* - type: 'commonjs'
* - main: 'lib/mainfile.js'
* - conditional exports for 'require' (lib/index.js) and
* 'import' (es/index.js)
* [2] - package.json add-on for the import case
* - it only contains:
* - type: 'module'
*
* in case the package is consumed as an ESM by importing it:
* import * as my-package from 'my-dual-package'
* it will cause the resolve method to return:
* {
* url: '<base_path>/node_modules/my-dual-package/es/index.js',
* format: 'module'
* }
*
* following testcase ensures that resolve works correctly in this case
* returning the information as specified above. Source for 'url' value
* is [1], source for 'format' value is [2]
*/
function testDualPackageWithJsMainScriptAndModuleType() {
// Create a dummy dual package
//
/**
* this creates following directory structure:
*
* ./node_modules:
* |-> my-dual-package
* |-> es
* |-> index.js
* |-> package.json [2]
* |-> lib
* |-> index.js
* |->package.json [1]
*
* [1] - main package.json of the package
* - it contains:
* - type: 'commonjs'
* - main: 'lib/mainfile.js'
* - conditional exports for 'require' (lib/index.js) and
* 'import' (es/index.js)
* [2] - package.json add-on for the import case
* - it only contains:
* - type: 'module'
*
* in case the package is consumed as an ESM by importing it:
* import * as my-package from 'my-dual-package'
* it will cause the resolve method to return:
* {
* url: '<base_path>/node_modules/my-dual-package/es/index.js',
* format: 'module'
* }
*
* following testcase ensures that resolve works correctly in this case
* returning the information as specified above. Source for 'url' value
* is [1], source for 'format' value is [2]
*/

const moduleName = 'my-dual-package';

const mDir = rel(`node_modules/${moduleName}`);
const esSubDir = rel(`node_modules/${moduleName}/es`);
const cjsSubDir = rel(`node_modules/${moduleName}/lib`);
const pkg = rel(`node_modules/${moduleName}/package.json`);
const esmPkg = rel(`node_modules/${moduleName}/es/package.json`);
const esScript = rel(`node_modules/${moduleName}/es/index.js`);
const cjsScript = rel(`node_modules/${moduleName}/lib/index.js`);

createDir(nmDir);
createDir(mDir);
createDir(esSubDir);
createDir(cjsSubDir);

const mainPkgJsonContent = {
type: 'commonjs',
main: 'lib/index.js',
exports: {
'.': {
'require': './lib/index.js',
'import': './es/index.js'
},
'./package.json': './package.json',
}
};
const esmPkgJsonContent = {
type: 'module'
};
const moduleName = 'my-dual-package';

fs.writeFileSync(pkg, JSON.stringify(mainPkgJsonContent));
fs.writeFileSync(esmPkg, JSON.stringify(esmPkgJsonContent));
fs.writeFileSync(esScript,
'export function esm-resolve-tester() {return 42}');
fs.writeFileSync(cjsScript,
`module.exports = {
esm-resolve-tester: () => {return 42}}`
);
const mDir = rel(`node_modules/${moduleName}`);
const esSubDir = rel(`node_modules/${moduleName}/es`);
const cjsSubDir = rel(`node_modules/${moduleName}/lib`);
const pkg = rel(`node_modules/${moduleName}/package.json`);
const esmPkg = rel(`node_modules/${moduleName}/es/package.json`);
const esScript = rel(`node_modules/${moduleName}/es/index.js`);
const cjsScript = rel(`node_modules/${moduleName}/lib/index.js`);

createDir(nmDir);
createDir(mDir);
createDir(esSubDir);
createDir(cjsSubDir);

const mainPkgJsonContent = {
type: 'commonjs',
main: 'lib/index.js',
exports: {
'.': {
'require': './lib/index.js',
'import': './es/index.js'
},
'./package.json': './package.json',
}
};
const esmPkgJsonContent = {
type: 'module'
};

fs.writeFileSync(pkg, JSON.stringify(mainPkgJsonContent));
fs.writeFileSync(esmPkg, JSON.stringify(esmPkgJsonContent));
fs.writeFileSync(esScript,
'export function esm-resolve-tester() {return 42}');
fs.writeFileSync(cjsScript,
`module.exports = {
esm-resolve-tester: () => {return 42}}`
);

// test the resolve
const resolveResult = resolve(`${moduleName}`);
assert.strictEqual(resolveResult.format, 'module');
assert.ok(resolveResult.url.includes('my-dual-package/es/index.js'));
}

function testDualPackageWithMjsMainScriptAndCJSType() {

// Additional test for following scenario
/**
* this creates following directory structure:
*
* ./node_modules:
* |-> dual-mjs-pjson
* |-> subdir
* |-> index.mjs [3]
* |-> package.json [2]
* |-> index.js
* |->package.json [1]
*
* [1] - main package.json of the package
* - it contains:
* - type: 'commonjs'
* - main: 'subdir/index.js'
* - conditional exports for 'require' (subdir/index.js) and
* 'import' (subdir/index.mjs)
* [2] - package.json add-on for the import case
* - it only contains:
* - type: 'commonjs'
* [3] - main script for the `import` case
*
* in case the package is consumed as an ESM by importing it:
* import * as my-package from 'dual-mjs-pjson'
* it will cause the resolve method to return:
* {
* url: '<base_path>/node_modules/dual-mjs-pjson/subdir/index.mjs',
* format: 'module'
* }
*
* following testcase ensures that resolve works correctly in this case
* returning the information as specified above. Source for 'url' value
* is [1], source for 'format' value is the file extension of [3]
*/
const moduleName = 'dual-mjs-pjson';

const mDir = rel(`node_modules/${moduleName}`);
const subDir = rel(`node_modules/${moduleName}/subdir`);
const pkg = rel(`node_modules/${moduleName}/package.json`);
const subdirPkg = rel(`node_modules/${moduleName}/subdir/package.json`);
const esScript = rel(`node_modules/${moduleName}/subdir/index.mjs`);
const cjsScript = rel(`node_modules/${moduleName}/subdir/index.js`);

createDir(nmDir);
createDir(mDir);
createDir(subDir);

const mainPkgJsonContent = {
type: 'commonjs',
main: 'lib/index.js',
exports: {
'.': {
'require': './subdir/index.js',
'import': './subdir/index.mjs'
},
'./package.json': './package.json',
}
};
const subdirPkgJsonContent = {
type: 'commonjs'
};

fs.writeFileSync(pkg, JSON.stringify(mainPkgJsonContent));
fs.writeFileSync(subdirPkg, JSON.stringify(subdirPkgJsonContent));
fs.writeFileSync(esScript,
'export function esm-resolve-tester() {return 42}');
fs.writeFileSync(cjsScript,
`module.exports = {
esm-resolve-tester: () => {return 42}}`
);

// test the resolve
const resolveResult = resolve(`${moduleName}`);
assert.strictEqual(resolveResult.format, 'module');
assert.ok(resolveResult.url.includes(`${moduleName}/subdir/index.mjs`));
}

testDualPackageWithJsMainScriptAndModuleType();
testDualPackageWithMjsMainScriptAndCJSType();

// test the resolve
const resolveResult = resolve(`${moduleName}`);
assert.strictEqual(resolveResult.format, 'module');
assert.ok(resolveResult.url.includes('my-dual-package/es/index.js'));
} finally {
process.chdir(previousCwd);
fs.rmSync(nmDir, { recursive: true, force: true });
Expand Down

0 comments on commit a149086

Please sign in to comment.