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

named imports/exports #5

Closed
Swatinem opened this issue Nov 2, 2015 · 10 comments

Comments

Projects
None yet
3 participants
@Swatinem
Copy link
Member

commented Nov 2, 2015

@Rich-Harris mentioned this already in #2 (comment) that named imports/exports are really difficult or even completely unfeasible.

The problem is that the current pattern is extremely popular, and established tools (even rollup) work well with it.

So far I use rollup with external to link to npm modules and it works like expected with named imports. Switching to rollup-plugin-commonjs however breaks because it now complains about missing exports (since the converted modules only have a default export and nothing else).

@Victorystick

This comment has been minimized.

Copy link
Member

commented Nov 2, 2015

What you mean is that you can no longer write

import { relative } from 'path';

since there doesn't exist any named exports from modules written in CommonJS?

@Swatinem

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2015

Yes exactly.

That pattern works fine when using the external option to just skip analyzing the import altogether. And it works with other tools such as webpack.

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2015

Have been thinking about this. We could add named exports in unambiguous exports.foo = 'bar' cases, alongside the default export:

var x = (function (module) {
  var exports = module.exports;

  /* the original code starts here */
  exports.foo = 'bar';
  module.exports.baz = 'qux'; // we detect both of these
  /* and ends here */

  return module.exports;
});

export default x;
export const foo = x.foo;
export const baz = x.baz;

That would reduce friction a bit. It wouldn't completely remove the inconsistency between internal vs external modules, because with external modules we have the luxury of waiting till runtime – so cases like this would fail...

function augment ( x ) {
  x.foo = 'bar';
  x.baz = 'qux';
}

augment( module.exports );

...but maybe that's okay.

@Swatinem

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2015

I would argue that the dynamic case (your augment) is not that common. A more common case is probably dynamic imports in commonjs (such as require(a ? './a' : './b') or even a = b.map(require))

So yes, it would be a good start to support the "static" use case and just error when there is some "dynamic" usage involved. Maybe prompting the user to switch to external to avoid such issues.

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2015

Just released 1.2.0 which implements this. Let me know if you run into problems with it

@Rich-Harris Rich-Harris closed this Nov 2, 2015

@Swatinem

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2015

Works like a charm! But now the need for caching/incremental compilation is even more evident, since now the rollup bundle time went from ~300ms to ~2s which is unbearable in production.

But very good job so far :-)

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2015

the rollup bundle time went from ~300ms to ~2s

woah – is that a like-for-like comparison, or is it just bundling a lot more files now?

@Swatinem

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2015

It is pulling in the whole of core-js and some other libs now.
Also I’m not using rollup-plugin-babel right now because with that the rebuild time would be more like 6s instead of ~300-600ms as it is now.

also the bundle size went doubled from 300k to 600k+ with all our external dependencies bundled in.

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

Also I’m not using rollup-plugin-babel right now because with that the rebuild time would be more like 6s

Is that true if you restrict it to your source files? (i.e. exclude: 'node_modules/**')?

@Swatinem

This comment has been minimized.

Copy link
Member Author

commented Nov 4, 2015

That is with only source files (before i tried the npm+commonjs plugins)

So the timing for our project is roughly:

  • ~300-600ms without plugins
  • ~4-6s with only babel (using external to only work on local source files)
  • ~2s with only npm/commonjs (without babel, which is done via gobble in a separate step)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.