Feat: add syntheticNamedExports

[manucorporat]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

Today named exports have to be known at compiler time in order to be imported within a different module:

import {foo, bar, thing, stuff} from './mod.js';

foo, bar, thing and stuff have to be known exports at compiler time so mod.js should look like this:

export const foo = () => {};
export const bar = 42;
export const thing = {};
export const stuff = [1,2,3,4];

However, there are cases where this is not always possible or optimal:

• The commonjs plugins performs a best-effort static analysis on top of commonjs code, in order to known at compiler time the "named" export of a cjs module. But this is not always possible, commonjs is highly dynamic and determined at runtime: circular dependencies or even creative ways of exporting things (https://github.com/npm/node-semver/blob/master/index.js#L7-L64) make it very difficult or impossible (requiring the plugin to have a namedExports option to workaround this limitation).

• Web assembly exported functions can not be infered at compiler time without MBs of native code making very hard to implement in rollup named exports of WASM modules:

import  { foo } from './module.wasm';

Same could be applied to languages like Rust:

import { operation } from  './module.rs';

This is big deal with we want rollup to follow the future standard way of importing WASM:
https://github.com/WebAssembly/esm-integration/tree/master/proposals/esm-integration

Proposed solution

This PR adds a new plugin-only option called syntheticNamedExports (name up to debate), that just like moduleSideEffects can be returned by the resolveId, load and transform hooks.

syntheticNamedExports is a boolean that allows rollup to fallback the resolution named exports to properties of the default export, example:

dep.js:

export const other = 'thing';
export default {
   foo: 42,
   bar: 'hello'
}

main.js

import {foo, bar, other} from './dep.js'
console.log(foo, bar, other);

Plugin

{
  resolveId(id) {
    if(id === './depjs') {
      return {id, syntheticNamedExports: true},
    }
    return null;
  }
}

Generated code:

const other = 'thing';
const dep = {
   foo: 42,
   bar: 'hello'
};

console.log(dep.foo, dep.bar, other);

Notice that even though dep.js does not export foo, bar, thanks to the new option, rollup is able to fallback the missing export to the property access of the default export.

Design notes

• syntheticNamedExports is false by default, preventing any kind of breaking change
• This should be a plugin-only option, not a global setting, so developers can propertly scope the impact of this new behaviour.

[codecov]

Codecov Report

Merging #3295 into master will increase coverage by 0.03%.
The diff coverage is 98.33%.

@@            Coverage Diff             @@
##           master    #3295      +/-   ##
==========================================
+ Coverage   93.12%   93.16%   +0.03%     
==========================================
  Files         170      171       +1     
  Lines        5966     6013      +47     
  Branches     1781     1794      +13     
==========================================
+ Hits         5556     5602      +46     
  Misses        219      219              
- Partials      191      192       +1

Impacted Files	Coverage Δ
src/ast/variables/NamespaceVariable.ts	100% <100%> (ø)	⬆️
src/Module.ts	97.47% <100%> (+0.16%)	⬆️
src/ast/variables/Variable.ts	96.15% <100%> (+0.32%)	⬆️
src/ModuleLoader.ts	98.52% <100%> (+0.05%)	⬆️
src/utils/transform.ts	95% <100%> (+0.19%)	⬆️
src/utils/error.ts	100% <100%> (ø)	⬆️
src/ast/variables/SyntheticNamedExportVariable.ts	87.5% <87.5%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7158c06...7d44c8d. Read the comment docs.

[lukastaegert]

Just a first look. I added a longer comment, the thing is I would prefer if we could change this feature to pick the properties off the namespace object on access instead to ensure live-binding and circular import support.

[manucorporat]

I had to add new code to handle this case:

import * as ns from './dep1.js';
console.log(ns['some-prop']);

to generate:

const d = {};
console.log(d["some-prop"]);

[shellscape]

@manucorporat since this has been moved out of draft and ready for review, could you please replace "WIP" with a solid description of the PR and what it adds? Lukas knows what's up but for the rest of the folks not familiar what synthetic named exports are.

[manucorporat]

@shellscape absolutely, thanks for the reminder, working on it!

[manucorporat]

@lukastaegert @shellscape updated the description! make let me know if it's clear enough, or it needs more explanation

[shellscape]

@manucorporat superb, thank you. 👍 for syntheticNamedExports as a name, very clear.

Should we create a small core plugin that utilizes this? Would that reduce adoption friction?

[manucorporat]

I think we could give it a try in commonjs-plugin first because it would solve a lot of problem there. We (at stencil) are also working in a much better web assembly plugin that would use this API and emitFile(), so we can load wasm using the most performant instantiateStreaming api.

But not sure, yet if this feature should be applied to all the modules as a global setting (or a core plugin that enabled it by default)

[shellscape]

Excellent. Not to get too far on a tangent, but please do consider putting that work toward @rollup/plugin-wasm rather than a new plugin. We don't currently have a champion for that core plugin and would welcome it.

[manucorporat]

@shellscape of course, we are happy to contribute it to the community but for the sake of iterating fast we are experimenting locally!
ionic-team/stencil#2093

import { method } from './a.wasm';

part of the design is to make it easy to integrate with language-specific plugins, like a rust one, so you can have:
rust.rs:

#[no_mangle]
pub extern fn foo(a: i32, b: i32) -> i32 {
  a + b
}

main.js:

import { foo } from './rust.rs';
foo(1, 2) // 3

[lukastaegert]

Looks really good. Just some minor suggestions. The last thing we need before release is to add documentation to the plugins section in the docs, but I could also help you with that once I am through the other reviews.

[lukastaegert]

This check should be superfluous as this.getDefaultExport throws when it cannot find an export. Therefore, the check if (syntheticExport) below is also unneeded and this can be simplified. Furthermore, the test for !this.exports[name] should always be true as otherwise, we would not get here.

[manucorporat]

@lukastaegert all done!

[lukastaegert]

Still wondering about this check. Shouldn’t L450-L456 make sure that this condition is always fulfilled because otherwise we would have done an early return there?

[manucorporat]

ups! done

[lukastaegert]

One minor thing: Maybe call this a ...Variable so that it fits the names of the other variable types

[manucorporat]

done!

[manucorporat]

Looks really good. Just some minor suggestions. The last thing we need before release is to add documentation to the plugins section in the docs, but I could also help you with that once I am through the other reviews.

Do you want to add that in this PR? or a different one?

[lukastaegert]

Documentation should ideally be part of the PR they document to avoid dependencies between PRs and make it easier to associate the two

[manucorporat]

Documentation should ideally be part of the PR they document to avoid dependencies between PRs and make it easier to associate the two

going to work on the docs today

[lukastaegert]

Awesome!

[lierdakil]

Sorry for a bit of a necroposting, but as far as I can tell, this feature doesn't help with the case of modules that have both named and default exports, e.g. in terms of CommonJS, something like

module.exports = {
  default: 'foo',
  helper: 'bar'
}

(this is basically what Babel did when converting ES6 exports to ES5, at least at some point)

Am I missing something?

EDIT: Sorry, I'm being an idiot, please disregard. I'll blame sleep deprivation.

[lukastaegert]

The assumption is that the default export is never a key named default or otherwise in such a situation, all actual named exports are detected by the plugin. The plugin still creates named exports that are separate from the default export if those are detectable, e.g. by using the patern exports.helper = 'bar'.
In the example you outlined it fails indeed. This might be an example where we would need to reintroduce the namedExports option.