Allow external CommonJS modules to be imported properly in Node
#3473
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/DwFjVSbxVJTSQGXH4TrMnqjvkk7t |
|
I might have been too eager here by making the ESM modules return the whole namespace. I'll change that part back. |
| 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)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
.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.
Changes
In Node anything that is
externalgoes throughrequireOrImport. For CommonJS this resolves to themodule.exportsvalue. In order to doimport def from 'default'that object needs adefaultvalue. This adds it.Testing
Test added.
Docs
N/A, bug fix.