-
Notifications
You must be signed in to change notification settings - Fork 116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good. Would be good to understand the one question.
Most of this is about require('x.mjs')
cases right? And dealing with the export default x
handling to work out ergonomically? Or have I misunderstood?
return `import * as ${name} from ${JSON.stringify(actualId)}; export default ${name};`; | ||
else if (esModulesWithDefaultExport[actualId]) { | ||
return `export {default} from ${JSON.stringify(actualId)};`; | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the logic for modules that neither don't have a default nor have a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They get a deoptimized wrapper. This is the case for
- modules that are ignored because of some
include
/exclude
pattern - modules that are ignored because they do not have one of the allowed extensions
The wrapper looks like this:
// before bundling
import * as foo from './foo.js';
export default getCjsExportFromNamespace(foo);
function getCjsExportFromNamespace(n) {
// as the property access is in a function, Rollup does not try to optimize `foo.default`
// which could result in a "default is not exported" warning
return n && n.default || n;
}
// after bundling
// the namespace from "foo.js"
var foo = /*#__PURE__*/Object.freeze({
namedExport: ...,
default: ...
});
function getCjsExportFromNamespace (n) {
return n && n.default || n;
}
// this is what is returned by require("./foo")
var require$$0 = getCjsExportFromNamespace(foo);
...
This is slightly less optimal than the previous version of the plugin which directly inserted the foo && foo.default || foo
without another function call. As noted in the comment above, however, this produced warnings in recent rollup versions. I assume the reason why previously, the default was accessed via foo['default']
was that some old version of rollup did not optimize this.
As the getCjsExportFromNamespace
is now shared between all wrappers, it is unlikely to be optimized by rollup in the future.
I am not 100% happy about the n && n.default || n
. This was the previous code. My feeling is that either
n && Object.prototype.hasOwnProperty.call(n, 'default') ? n.default : n
or
n && ('default' in n) ? n.default : n
would produce better results, but I was not sure about all implications.
Yes exactly. In case there is a known default export, the generated code will be much shorter. There are still quite a few cases where the deoptimized wrapper is used. I have an idea how this could be solved while making the CJS detection much more straightforward at the same time. This would require a new plugin context function, e.g. |
Besides making the tests ready for rollup@1.0.0 (the production code was already working), this will also fix two issues:
export default ...
as well asexport {... as default}
.Furthermore, it prevents a
default is not exported
warning by putting the ESM import wrapper into a function which is not optimized by rollup. Lastly, importing ESM with known default exports will produce more optimized code.