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

Rollup's external option only sufficient to suppress import, skip additionally required to suppress require #72

Closed
wearhere opened this issue Jan 9, 2017 · 9 comments

Comments

@wearhere
Copy link

wearhere commented Jan 9, 2017

UPDATE 07.13.2017: This issue is written as if we still had the skip option from version 2.x. As this comment notes, 3.x does not fix the problem with this module and external but rather removes the workaround of specifying skip. :(


As per @Rich-Harris's comment here, this plugin will automatically skip modules marked as external (without the user having to specify the skip option). But, my testing reveals that the plugin will only do this if the module is imported using an import statement, not via a require statement—in the latter case, you must also specify skip.

Example:

// index.js
import {getFirstName} from './StringUtils.js';
console.log(getFirstName('Jeff Wear'));
// StringUtils.js
var _ = require('underscore');

module.exports.getFirstName = function(name) {
  // Slightly contrived I know
  return _.first(name.split(' '));
};
// package.json
{
  "devDependencies": {
    "rollup": "^0.45.2",
    "rollup-plugin-commonjs": "^8.0.2",
    "rollup-plugin-node-resolve": "^3.0.0"
  },
  "dependencies": {
    "underscore": "^1.8.3"
  }
}

The following Rollup configuration will not prevent Underscore from being bundled:

export default {
  entry: 'index.js',
  external: ['underscore'],
  plugins: [
    nodeResolve(),
    commonJS(),
  ],
  targets: [{
    format: 'iife',
    dest: 'bundle.js'
  }]
};

—I must also specify nodeResolve({skip: ['underscore']}). (skip alone is not sufficient—if I don't define external, Rollup prints a "Treating 'underscore' as external dependency" warning.)

However, if I make StringUtils an ES6 module:

// StringUtils.js
import _ from 'underscore';

export function getFirstName(name) {
  return _.first(name.split(' '));
}

Then external: ['underscore'] is sufficient.

Is this expected? It would be nice if external was sufficient for both ES6 and CommonJS modules.

It's also kind of weird that I need both skip and external in the CommonJS case. EDIT: Upon re-reading this comment I take this back since even if we get rollup-plugin-node-resolve to skip resolution we might not want to treat the dependency as totally external but rather want Rollup to resolve it some other way.

@wearhere wearhere changed the title Rollup's external option only applies to import statements, skip required to suppress require Rollup's external option only sufficient to suppress import, skip additionally required to suppress require Jan 9, 2017
@unional
Copy link

unional commented Jan 13, 2017

Just external does not do it for named import either:

import { set } from 'lodash'
export { set }

bundles lodash if without skip

@fregante
Copy link

I assume this was solved by #90. If not, leave a comment

@jochenberger
Copy link

I think that #90 did not solve the issue but rather removed the workaround, because it is no longer possible to specify skip in addition to external.

@wearhere
Copy link
Author

wearhere commented Jul 14, 2017

Whoever fixes this, please note the following quirk in the previous behavior of skip (a bug that the new implementation should avoid!)

Consider this setup using [email protected]:

// index.js
import {doSomethingWithHandlebars} from './StringUtils.js';
console.log(doSomethingWithHandlebars());
// StringUtils.js
var Handlebars = require('handlebars/runtime');
module.exports.doSomethingWithHandlebars = function() {
  Handlebars.madeUpFunction();
};
// package.json
{
  "dependencies": {
    "rollup": "^0.45.2",
    "rollup-plugin-commonjs": "^8.0.2",
    "rollup-plugin-node-resolve": "^2.0.0"
  },
  "dependencies": {
    "handlebars": "^4.0.10",
  }
}
// rollup.config.js
import nodeResolve from 'rollup-plugin-node-resolve';
import commonJS from 'rollup-plugin-commonjs';

export default {
  entry: 'index.js',
  external: ['handlebars/runtime'],
  plugins: [
    nodeResolve({
      skip: ['handlebars/runtime']
    }),
    commonJS(),
  ],
  targets: [{
    format: 'iife',
    dest: 'bundle.js'
  }]
};

The above skip list will not exclude Handlebars from the bundle!

Instead you have to do skip: ['handlebars']. That is, it appears that skip's values must be the canonical package names even if you're require'ing a subpath.

But hopefully the fix to this issue will allow external: ['handlebars/runtime'] to be sufficient.

wearhere added a commit to wearhere/rollup-plugin-node-resolve-globals that referenced this issue Jul 14, 2017
@DanielJoyce
Copy link

DanielJoyce commented Mar 9, 2018

External doesn't work at all for me. It always pulls in a relative path if it is found in node_modules.

@lili21
Copy link

lili21 commented Dec 5, 2018

any updates ?

@lili21
Copy link

lili21 commented Dec 5, 2018

3.x does not fix the problem with this module and external but rather removes the workaround of specifying skip. :(

what happened ?

@dima-takoy-zz
Copy link

any updates?

@dima-takoy-zz
Copy link

I checked it with latest versions, all works fine, with commonjs module and with es6 module. Repro here: https://github.com/mecurc/72-rollup-resolve

Can be closed now.

@TrySound TrySound closed this as completed May 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants