Plugin order #1148

Open
Rich-Harris opened this Issue Dec 14, 2016 · 13 comments

Projects

None yet

5 participants

@Rich-Harris
Contributor

A couple of recent issues (rollup/rollup-plugin-commonjs#145 and rollup/rollup-plugin-babel#58) have been caused by the order in which plugins run. It's not obvious that, for example, the commonjs plugin needs to run before the babel plugin, or that node-resolve has to come before commonjs.

I'm not sure what the best solution is. Perhaps plugins could export an order property that allows Rollup to warn if it looks like plugins are in the wrong order?

function myPlugin ( options ) {
  return {
    name: 'my-plugin',

    order: {
      commonjs: -1,
      babel: 1
    },

    transform ( code, id ) {
      // ...
    }
  };
}

Or something more intuitive, like mustFollow: [ 'commonjs' ] and mustPrecede: [ 'babel' ].

This wouldn't catch every case, but it would allow us to warn on the common cases. Thoughts?

@kzc
Contributor
kzc commented Dec 14, 2016

mustFollow: [ 'commonjs' ] and mustPrecede: [ 'babel' ]

That's probably the simplest solution.

Might there be a valid use case to repeat plugins within a rollup config? If so, that might present a complication. But even then a warning would be emitted and execution could continue.

@Rich-Harris
Contributor

Might there be a valid use case to repeat plugins within a rollup config?

Conceivably, yes โ€“ you might want to transpile some files with these Babel options and some with those Babel options, for example. Typically though you would probably want to have those plugin instances next to each other, I think. And yeah, it's just a warning rather than an error.

@kzc
Contributor
kzc commented Dec 14, 2016

Are there rollup options to disable warnings and/or error out upon warning?

@Rich-Harris
Contributor

You can supply an options.onwarn handler, which you can use to suppress or throw. It will default to console.warning message. Right now warnings are just strings, I'd like to make them a bit more structured eventually

@Rich-Harris Rich-Harris referenced this issue in rollup/rollup-plugin-commonjs Dec 29, 2016
Open

Unable to access the exports of 3rd-party module #154

@Rich-Harris
Contributor

Right now warnings are just strings, I'd like to make them a bit more structured eventually

#1194

@btd
Contributor
btd commented Jan 12, 2017

I do not think adding additional keys to show plugin run after is right idea.
Plugins will just start work in any order - no body will know what is going on.
We completly loose obviosity.

PostCSS has similar problem, but they wrote specific order steps if required in readme. So all problems are comming from users that did not read readme.

@Rich-Harris
Contributor

The proposal isn't that plugins would control the order in which they run, just that they'd provide hints that would allow Rollup to say 'by the way, this config probably isn't going to work, you should probably make sure that the commonjs plugin goes before the babel plugin. But YMMV'.

@jharris4
jharris4 commented Jan 29, 2017 edited

I'm confused by the statement above It's not obvious that, for example, the commonjs plugin needs to run before the babel plugin.

I just tried converting a project to use rollup that's using babel-plugin-transform-object-rest-spread.

If I use the following rollup plugin ordering:

plugins: [
  replace({
    'process.env.NODE_ENV': JSON.stringify('production')
  }),
  node(),
  commonjs(),
  babel(babelConfig)
]

I get an error from the commonjs plugin:

๐Ÿšจ   (commonjs plugin) Unexpected token (<line>:<col>) in <filename.js>

Where the corresponding source looks like:

let myVar = functionCall({...objectToSpread, otherArg});

The only way I can fix the error is to change the order of the plugins such that babel is before commonjs, like so:

plugins: [
  replace({
    'process.env.NODE_ENV': JSON.stringify('production')
  }),
  node(),
  babel(babelConfig),
  commonjs()
]

Am I missing something here? I don't quite understand why commonjs is supposed to be before babel, or how to workaround the above issue.

@btd
Contributor
btd commented Jan 30, 2017

@jharris4 I think this is because you use object spread, which is not in any es standard yet. But commonjs plugin uses acorn which supports only stage-4 items (which is in standard actually).

@mortargrind

@btd Shouldn't Rollup let the rollup-user to use additional acorn plugins (for things like JSX or Object spread) if needed in order to avoid syntax errors during parsing stage? Because it seems that current Rollup plugin order requires us to use Babel at the very end of the flow. But if the rollup-user uses anything that adds special syntax to the ES itself, Acorn can not parse it in the first place so we are forced to use Babel first in the plugin order: Most of the examples about React+Rollup are like this and they're actually wrong. It literally took more than 1 day for me to figure out it's about the plugin order and Acorn :(

I know that the decision was to wait for the community to grow and develop Rollup transform plugins, but at this stage if the Acorn plugin ecosystem is more rich and stable i think it should be an option to use Acron plugins directly.

@btd
Contributor
btd commented Feb 3, 2017 edited

@mortargrind i do not think so. Each plugin distinct from rollup itself, and it is literally your responsobility currently to provide plugin source it could work with (that is why this issue raised - to make this process more simple).
If we are trying to go this road (allow acorn plugins) - each plugin should have tons of additional options. I do not think it is right idea. But that is only my opinion.
That is why babel should be first to convert source to es5 (but actually most plugins allow es6+ stage-4 things) and pass it further. So just choose format you want to be at the end. I usually need es5 for browsers we support.
For example one of the projects uses react and rollup has such config:

function scripts(entry, cache) {

  return rollup({
    entry: entry,
    sourceMap: sourceMap,
    format: 'iife',
    cache: cache,
    plugins: [
      rollupJson(),
      rollupBabel({
        plugins: [
          "external-helpers",
        ],
        presets: [
          ['es2015', { loose: true, modules: false }],
          'react'
        ],
        exclude: 'node_modules/**'
      }),
      rollupReplace({
        'process.env.NODE_ENV': JSON.stringify(isDev ? 'development': 'production'),
        'process.env.VERSION': JSON.stringify(pkg.version),
        'process.env.SITE_URL': JSON.stringify(config.SITE_URL),
        'process.env.CDN': JSON.stringify(config.CDN),
        'process.env.PATH': JSON.stringify(config.PATH)
      }),
      rollupNodeResolve({
        extensions: ['.js', '.jsx'],
        jsnext: true,
        main: true
      }),
      rollupCommonJs({
        ignoreGlobal: true,
        include: 'node_modules/**',
        namedExports: {
          'node_modules/react/react.js': ['Children', 'Component', 'PropTypes', 'createElement']
        }
      }),
      rollupVisualizer({
        filename: './stats.html'
      })
    ]
  });
}

(it is part of gulpfile so additional options)
I do not see anything wrong with plugin order at all.

@mortargrind
mortargrind commented Feb 3, 2017 edited

@btd So according to this, no need to run babel at the end (after common-js) at all? If so, where should we put "globals" and "builtins" plugins in your example? I'm still getting different error messages according to their order too. Sorry for not asking this in a seperate issue, i was going to, but i think their order may also related to babel/common/nodejs plugins order.

@btd
Contributor
btd commented Feb 4, 2017 edited

@mortargrind yes there is no any reason to run babel after commonjs. commonjs plugin do not add anything to source file that other plugins could not consume (it adds in most situations imports and exports, which rollup handle itself).
I could be wrong there (i do not use globals and builtins feature), but commonjs only could handle global var usage and wrap file in iife. Builtins i am not sure how handled, there is empty repo. Found this plugin

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