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

Allow external CommonJS modules to be imported properly in Node #3473

Merged
merged 3 commits into from Jun 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion esinstall/src/index.ts
Expand Up @@ -373,7 +373,8 @@ ${colors.dim(
warning.code === 'CIRCULAR_DEPENDENCY' ||
warning.code === 'NAMESPACE_CONFLICT' ||
warning.code === 'THIS_IS_UNDEFINED' ||
warning.code === 'EMPTY_BUNDLE'
warning.code === 'EMPTY_BUNDLE' ||
warning.code === 'UNUSED_EXTERNAL_IMPORT'
) {
logger.debug(logMessage);
} else {
Expand Down
18 changes: 15 additions & 3 deletions snowpack/assets/require-or-import.js
Expand Up @@ -20,16 +20,28 @@ module.exports = async function requireOrImport(filepath, { from = process.cwd()
});
try {
const mdl = NATIVE_REQUIRE(resolvedPath);

// Add a `default` property on a CommonJS export so that it can be accessed from import statements.
// This is set to enumerable: false to make it hidden(ish) to code.
if(mdl && !('default' in mdl)) {
Object.defineProperty(mdl, 'default', {
configurable: true,
enumerable: false,
writable: true,
value: mdl
});
}

resolve(mdl);
} catch (e) {
if (e instanceof SyntaxError && /export|import/.test(e.message)) {
console.error(`Failed to load "${filepath}"!\nESM format is not natively supported in "node@${process.version}".\nPlease use CommonJS or upgrade to an LTS version of node above "node@12.17.0".`)
} else if (e.code === 'ERR_REQUIRE_ESM') {
const url = pathToFileURL(resolvedPath);
return NATIVE_IMPORT(url).then(mdl => resolve(mdl.default ? mdl.default : mdl));
};
return NATIVE_IMPORT(url).then(mdl => resolve(mdl.default ? mdl.default : mdl), err => reject(err));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any difference between onRejected vs catch()? I have a slight preference for the latter because it’s easier to read but don’t feel too strongly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.catch matters where you put it, if you put it after the then it would reject if the caller had an error, when is not what we want. Could have put the .catch before the then though.

}
try {
return NATIVE_IMPORT(pathToFileURL(resolvedPath)).then(mdl => resolve(mdl.default ? mdl.default : mdl));
return NATIVE_IMPORT(pathToFileURL(resolvedPath)).then(mdl => resolve(mdl.default ? mdl.default : mdl), err => reject(err));
} catch (e) {
reject(e);
}
Expand Down
45 changes: 45 additions & 0 deletions test/snowpack/runtime/runtime.test.js
Expand Up @@ -90,4 +90,49 @@ describe('runtime', () => {
await fixture.cleanup();
}
});

it('Can import a CommonJS module as the default export', async () => {
const fixture = await testRuntimeFixture({
'packages/other/package.json': dedent`
{
"version": "1.0.0",
"name": "other",
"main": "main.js"
}
`,
'packages/other/main.js': dedent`
module.exports = () => 'works';
`,
'main.js': dedent`
import fn from 'other';

export function test() {
return fn();
}
`,
'package.json': dedent`
{
"version": "1.0.1",
"name": "@snowpack/test-runtime-import-cjs",
"dependencies": {
"other": "file:./packages/other"
}
}
`,
'snowpack.config.json': dedent`
{
"packageOptions": {
"external": ["other"]
}
}
`
});

try {
let mod = await fixture.runtime.importModule('/main.js');
expect(await mod.exports.test()).toEqual('works');
} finally {
await fixture.cleanup();
}
});
});