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

fix: the empty string importee should not always be treated as external module id #2733

Merged
merged 2 commits into from Mar 4, 2019
Merged

fix: the empty string importee should not always be treated as external module id #2733

merged 2 commits into from Mar 4, 2019

Conversation

LongTengDao
Copy link
Contributor

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

I'm not sure which category to put test about this bug.

@lukastaegert
Copy link
Member

Thanks for your contribution. Could you explain, in what situation the code you changed caused a bug and what the nature of the bug was?

The best category to add a test to is usually the function category unless the bug is specific to a format other than CommonJS. function tests support all kinds of options, actually run the created code (including code-splitting situations) and have a way range of additional checks that can be added. If you want, I can help you with adding a test.

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Mar 3, 2019

Thanks for your contribution. Could you explain, in what situation the code you changed caused a bug and what the nature of the bug was?

Sure! When we pack this:

// rollup-import.js#.js
export var importee = 'xxx';

// rollup-import.js
import { importee } from '';
export var importer = importee;

// rollup-input.js
import { importer } from './rollup-importer.js';
export default importer;

(using this:)

const { rollup } = require('rollup');

( async function () {
	
	const bundle = await rollup({
		input: __dirname+'/rollup-input.js',
		external: () => false,
		plugins: [{
			resolveId (importee, importer) {
				if ( importee==='' ) { return importer+'#.js'; }
			}
		}]
	});
	
	const { output } = await bundle.generate({
		format: 'cjs',
		interop: false,
		exports: 'default'
	});
	
	const [{ code }] = output;
	
	console.log(code);
	
} )().catch(console.error);

We will always got this:

'use strict';

var  = require('');

var importer = .importee;

module.exports = importer;

Because after custom external check return false, the rollup engine will always check the module id with rawCommandOptions.external.includes (maybe for -e foo,bar,baz? which should be empty when use js api, but "" left), and then the "" was treated as an external module id (and it's variable name...)

var getExternal = function (config, command) {
    var configExternal = config.external;
    return typeof configExternal === 'function'
        ? function (id) {
            var rest = [];
            for (var _i = 1; _i < arguments.length; _i++) {
                rest[_i - 1] = arguments[_i];
            }
            return configExternal.apply(void 0, [id].concat(rest)) || command.external.indexOf(id) !== -1;
        }
        : (configExternal || []).concat(command.external);
};

After my fix, we will got:

'use strict';

var importee = 'xxx';

var importer = importee;

module.exports = importer;

The best category to add a test to is usually the function category unless the bug is specific to a format other than CommonJS. function tests support all kinds of options, actually run the created code (including code-splitting situations) and have a way range of additional checks that can be added. If you want, I can help you with adding a test.

I think I really need... thank you. After this time, I think I can learn how to write the test correctly...
BTW: this problem is also existing in esm, amd and iife, and will be not existing any more after this fix.

@lukastaegert
Copy link
Member

I have added a simple test based on your example, please have a look. I'll add some comments to explain how it works.

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Mar 4, 2019

@lukastaegert
Thank you!! I think the test is indeed what we need here. Test is so easy under your explaining!

@lukastaegert lukastaegert merged commit 6010bdd into rollup:master Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants