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

Add named exports for all plugins #360

Closed
NotWoods opened this issue May 1, 2020 · 19 comments
Closed

Add named exports for all plugins #360

NotWoods opened this issue May 1, 2020 · 19 comments

Comments

@NotWoods
Copy link
Member

NotWoods commented May 1, 2020

  • Rollup Plugin Name: all plugins except for node-resolve and babel

Feature Use Case

Using the default export creates issues with CommonJS if we want to export more than one item. When using @rollup/plugin-babel, the different import styles look like:

import babel from '@rollup/plugin-babel';
const babel = require('@rollup/plugin-babel').default;

In contrast, the named exports look more similar:

import { babel } from '@rollup/plugin-babel';
const { babel } = require('@rollup/plugin-babel');

Note: both formats work today because we export the plugin under babel and default.

Feature Proposal

As soon as we add any additional exports to a module, as was the case for @rollup/plugin-node-resolve, we need to use named exports. I think we should switch to named exports for all plugins for consistency. We can continue to export default as well for the ES module import format, but CJS outputs would have breaking changes.

@LarsDenBakker
Copy link
Contributor

I am in favor named exports, they create less friction and allow future additions.

@shellscape
Copy link
Collaborator

I'd recommend doing this one PR at a time. While tedious, easier to manage the releases and scheduling.

@TrySound
Copy link
Member

TrySound commented May 1, 2020

Hooray!

@lukastaegert
Copy link
Member

lukastaegert commented May 2, 2020

I thought this would make at least one person happy. And I see the need for babel. But I am not happy doing this unforced for the other plugins

  1. It will be a breaking change for each plugin
  2. Command line plugin usage will no longer work for many plugins

To explain (2):
At the moment, you can

rollup -i input.js -f es -p @rollup/plugin-node-resolve

and

rollup -i input.js -f es -p node-resolve

Now the problem is for Rollup to find the relevant export. So it requires the plugin and then

  1. if it is an object, use the plugin name as a property and call it (this needed to be added to make terser work…)
  2. otherwise call the plugin itself (which only works for default exports)

For 1., at the moment it would try plugin["@rollup/plugin-node-resolve"] in the first case and plugin["node-resolve"] in the second case. None of this would work.

So if you do this, we need to adjust the logic in Rollup first. PR very much welcome, the logic is very localized in https://github.com/rollup/rollup/blob/master/cli/run/commandPlugins.ts#L66-L73

And then once this is done, Rollup's documentation should be updated quickly, otherwise I see a ton of issues rolling in.

@TrySound
Copy link
Member

TrySound commented May 2, 2020

Babel will not work with cli anyway. We can introduce some other solution like special export

export const plugin = () => ({ name: 'plugin' })
export const specialExportWithPluginForCli = plugin

@lukastaegert
Copy link
Member

Why would Babel not work?

@lukastaegert
Copy link
Member

And technically, there should be no reason to have a separate plugin for CLI. CLI even supports plugin parameters. One easy approach could be to keep the default exports as well as the named export and have Rollup CLI check the default property as well as the entire export.

@NotWoods
Copy link
Member Author

NotWoods commented May 2, 2020

I'd like to keep default exports anyways to reduce the level of breakage. I'll put up a PR for Rollup so it checks for default exports.

We can phase this change in slowly and even wait on some PRs until we have other breaking changes to include.

@shellscape
Copy link
Collaborator

So we've got two really valid use cases here.

  1. We need the ability to export, or provide to the user, more than just the default export.
  2. We need the default export to support things like loading via CLI

There is a pattern that solves both, even though it's not a purist pattern. In the case of node-resolve where we wanted to export the default options, we could have gone a different route:

const defaultOptions = {};
const plugin = () => {};

plugin.defaults = Object.freeze(Object.assign({}, defaultOptions));

export default plugin;

While it's not as clean as named exports, it works. I've seen it a lot recently. Namely working with PouchDB and Electron. We can also attach types to the default export via namespace so we end up with something like:

export interface MainDatabase {
  config: PouchDB.Database;
  users: PouchDB.Database;

  sync: (uri: string) => void;
}

I think the case for default export for use on the CLI is strong. Users had clamored for that for a long while and just recently got that in. I think we can meet the needs of both use cases without causing too much disruption.

@NotWoods
Copy link
Member Author

NotWoods commented May 2, 2020

I'm against placing all the exports on the default function for the ES module build. We could do it for the CommonJS build but it would complicate the typings, and I think named exports is more straightforward.

@shellscape
Copy link
Collaborator

I suppose I'm still hunting for the cost/benefit here. We're talking about a lot of changes and adding additional complexity to the ecosystem by making these changes. If we're going to move forward with named exports on every plugin then there needs to be clear benefit that outweighs the complexity.

@tivac
Copy link

tivac commented May 2, 2020

What is the advantage of this relatively large change? Export purity seems like a terrible argument for all of this work/churn.

@lukastaegert
Copy link
Member

I think @shellscape's should actually work quite well. What we could do is

  • use a default export for the actual plugin

  • use named exports for everything else (we can also export the plugin twice, once as the default and once named if that is what we want)

  • do some magic for the CJS build, e.g. like this:

    // rollup.config.js
    export default {
      input: 'src/index.js',
      onwarn(warning, warn) {
          if (warning.code !== 'MIXED_EXPORTS') {
              warn(warning);
          }
      },
      output: {
          format: 'cjs',
          dir: 'dist',
          footer: 'module.exports = Object.assign(exports.default, exports);'
      }
    }

    This would automatically attach all named exports to the default export and use that as the only export. Thus we could have a single build and a somewhat consistent interface when switching from CJS to ESM.

@vladshcherbin
Copy link

Hopefully this won't land for plugins with single export. After years of using default exports this is a terrible proposal with no real advantages.

@dilyanpalauzov
Copy link

dilyanpalauzov commented May 11, 2020

Why not switching instead everything to ES6 (and abolish require())? Then CommonJS cannot make problems.

It is in first place about replacing in the documentation require() with import. This shall happen soon or later.

@kzc
Copy link

kzc commented May 16, 2020

-1 on dropping default export for plugins. If plugin maintainers wish to add an additional redundant named export - that's fine. But breaking existing plugins with default export just for questionable API elegance doesn't make sense.

I'm glad to see that export default was reinstated in node-resolve: fe037db.

As mentioned above, the rollup CLI --plugin option will have a problem identifying the entry point for plugins with names containing a dash (-) if they lack default export. In commonjs it is legal to have named exports with dashes using the subscript notation, but dashes are not valid in an export symbol in ES6. I see that the camelCase nodeResolve was chosen as the export name for node-resolve, but it just as easily could have been the snake_case node_resolve or even the no-dash noderesolve. This plugin naming confusion is unnecessary with default export.

@NotWoods
Copy link
Member Author

This issue is not proposing that we drop default exports, but instead export both named and default exports. ES module files will continue to work exactly the same, this only affects CommonJS. With Lukas' proposed solution we can make this a non-breaking change.

@kzc
Copy link

kzc commented May 17, 2020

If that's the case you might consider revising the issue title to reflect that. "Switch to" implies dropping the existing export mechanism and replacing it with a new one. "Add" would be a more apt description.

@NotWoods NotWoods changed the title Switch to named exports for all plugins Add named exports for all plugins May 17, 2020
@stale stale bot added the x⁷ ⋅ stale label Jul 16, 2020
@stale
Copy link

stale bot commented Jul 20, 2020

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.# Comment to post when closing a stale issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants