Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Prepare for 1.0.0, fix some issues #349

Merged
merged 4 commits into from Oct 10, 2018
Merged

Prepare for 1.0.0, fix some issues #349

merged 4 commits into from Oct 10, 2018

Conversation

lukastaegert
Copy link
Member

Besides making the tests ready for rollup@1.0.0 (the production code was already working), this will also fix two issues:

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.

Copy link
Contributor

@guybedford guybedford left a 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
Copy link
Contributor

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?

Copy link
Member Author

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.

@lukastaegert
Copy link
Member Author

Most of this is about require('x.mjs') cases right? And dealing with the export default x handling to work out ergonomically?

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. fetchModule, which would allow the transform function to trigger loading, transforming and parsing a module the same way Rollup would (or just return the result if this has already happened) and return the AST (if the result is stored by Rollup, each module would still be fetched and transformed only once). If this is done for every import in a CJS file, we can decide for all but the external imports whether they have a default and produce much better code. But this is just an idea for now which would require quite a bit of work on Rollup's side.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants