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

introduce CLI --plugin support #3379

Merged
merged 7 commits into from
Feb 28, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/help.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Basic options:
-m, --sourcemap Generate sourcemap (`-m inline` for inline map)
-n, --name <name> Name for UMD export
-o, --file <output> Single output file (if absent, prints to stdout)
-p, --plugin <name> Use plugin specified. May be repeated.
-v, --version Show version number
-w, --watch Watch files in bundle and rebuild on changes
--amd.id <id> ID for AMD module (default is anonymous)
Expand Down
63 changes: 62 additions & 1 deletion cli/run/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { realpathSync } from 'fs';
import relative from 'require-relative';
import { WarningHandler } from '../../src/rollup/types';
import { InputOptions, WarningHandler } from '../../src/rollup/types';
import mergeOptions, { GenericConfigObject } from '../../src/utils/mergeOptions';
import { getAliasName } from '../../src/utils/relativeId';
import { handleError } from '../logging';
Expand Down Expand Up @@ -118,7 +118,68 @@ async function execute(
if (command.stdin !== false) {
inputOptions.plugins!.push(stdinPlugin());
}
if (command.plugin) {
const plugins = Array.isArray(command.plugin) ? command.plugin : [command.plugin];
for (const plugin of plugins) {
if (/[={}]/.test(plugin)) {
// -p plugin=value
// -p "{transform(c,i){...}}"
loadAndRegisterPlugin(inputOptions, plugin)
} else {
// split out plugins joined by commas
// -p node-resolve,commonjs,buble
plugin.split(",").forEach((plugin: string) => loadAndRegisterPlugin(inputOptions, plugin));
}
}
}
await build(inputOptions, outputOptions, warnings, command.silent);
}
}

function loadAndRegisterPlugin(inputOptions: InputOptions, pluginText: string) {
let plugin : any = null;
let pluginArg : any = undefined;
if (pluginText[0] === '{') {
// -p "{transform(c,i){...}}"
plugin = new Function('return ' + pluginText);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

} else {
const match = pluginText.match(/^([@.\/\\\w|^{}|-]+)(=(.*))?$/);
if (match) {
// -p plugin
// -p plugin=arg
pluginText = match[1];
pluginArg = new Function('return ' + match[3])();
} else {
throw new Error(`Invalid --plugin argument format: ${JSON.stringify(pluginText)}`);
}
if (!/^\.|[@\/\\]/.test(pluginText)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I completely understand this regexp. Why would I want to try adding prefixes to a path that starts with a . but contains @ or slashes? Why not just if (!pluginText.startsWith('.'))...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've missed the or. Only trying the plugin prefixes if it does not start with a dot, or if it does not contain an @ or slashes - only node resolve should be used in those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon re-reading your comments, you may have missed the leading ! in the condition. Although this if is not strictly needed, it is an optimization to avoid expensive file system checks in cases where a prefix is not possible such as "foo/bar" or "../baz". Relative paths and any path with a slash or @ will never be a prefix candidate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it is even worse, I saw the ! but forgot how basic logic works when I negate an OR, don't ask 🙄

// Try using plugin prefix variations first if applicable.
// Prefix order is significant - left has higher precedence.
for (const prefix of ['@rollup/plugin-', 'rollup-plugin-']) {
if (!RegExp(prefix).test(pluginText)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only relevant case here is that the pluginText actually starts with one of the prefixes. Also, it makes no sense to try to require @rollup/plugin-rollup-plugin-node-resolve or rollup-plugin-@rollup/plugin-node-resolve. So why not change the if-statement in line 155 to

if (!['.', '@rollup/plugin-', 'rollup-plugin-'].some(prefix => pluginText.startsWith(prefix)) { //…

and get rid of the second if.

On further thought, it makes sense IMO to always treat a plugin starting with a . as a relative path, so we could treat this case earlier do not branch here if this case applies. Then we could extract ['@rollup/plugin-', 'rollup-plugin-'] as a constant both from the loop and the if-statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to take over this PR as long as all the tests pass. The functionality is sufficient for my own projects. The idea was that plugins should use node resolve unless they were relative or absolute paths. The if in question prevents an unnecessary file system lookup in cases were the path is relative or absolute. If you can accomplish that through other simpler means, that's fine too.

try {
plugin = require(prefix + pluginText);
break;
} catch (ex) {
// do nothing
}
}
}
}
if (!plugin) {
try {
if (pluginText[0] == ".") pluginText = process.cwd() + "/" + pluginText;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just path.resolve(pluginText)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can get it to work, sure, please do so. I only want plugins with relative paths to work from the current working directory from which the command was run.

plugin = require(pluginText);
} catch (ex) {
throw new Error(`Cannot load plugin '${pluginText}'`);
}
}
}
if (typeof plugin === 'object' && pluginText in plugin) {
// some plugins do not use `export default` for their entry point.
// attempt to use the plugin name as the named import name.
plugin = plugin[pluginText];
}
inputOptions.plugins!.push(typeof plugin === 'function' ? plugin.call(plugin, pluginArg) : plugin);
}
}
2 changes: 2 additions & 0 deletions src/utils/mergeOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export const commandAliases: { [key: string]: string } = {
m: 'sourcemap',
n: 'name',
o: 'file',
p: 'plugin',
v: 'version',
w: 'watch'
};
Expand Down Expand Up @@ -158,6 +159,7 @@ export default function mergeOptions({
Object.keys(commandAliases),
'config',
'environment',
'plugin',
'silent',
'stdin'
),
Expand Down
5 changes: 5 additions & 0 deletions test/cli/samples/plugin/absolute/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = {
description: 'CLI --plugin /absolute/path',
skipIfWindows: true,
command: `echo 'console.log(VALUE);' | rollup -p "\`pwd\`/my-plugin={VALUE: 'absolute', ZZZ: 1}"`
};
1 change: 1 addition & 0 deletions test/cli/samples/plugin/absolute/_expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log("absolute");
14 changes: 14 additions & 0 deletions test/cli/samples/plugin/absolute/my-plugin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
module.exports = function(options) {
if (options === void 0) options = {};
return {
transform(code) {
// dumb search and replace for test purposes
for (var key in options) {
const rx = new RegExp(key, 'g');
const value = JSON.stringify(options[key]);
code = code.replace(rx, value);
}
return code;
}
};
};
5 changes: 5 additions & 0 deletions test/cli/samples/plugin/advanced/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = {
description: 'advanced CLI --plugin functionality with rollup config',
skipIfWindows: true,
command: `rollup -c -p node-resolve,commonjs -p "terser={output: {beautify: true, indent_level: 2}}"`
};
17 changes: 17 additions & 0 deletions test/cli/samples/plugin/advanced/_expected/cjs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
"use strict";

Object.defineProperty(exports, "__esModule", {
value: !0
});

var t = function() {
function t(t) {
this.x = t;
}
return t.prototype.output = function() {
var t;
t = this.x, console.log(t);
}, t;
}();

new t(123).output(), exports.Bar = t;
13 changes: 13 additions & 0 deletions test/cli/samples/plugin/advanced/_expected/es.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var t = function() {
function t(t) {
this.x = t;
}
return t.prototype.output = function() {
var t;
t = this.x, console.log(t);
}, t;
}();

new t(123).output();

export { t as Bar };
4 changes: 4 additions & 0 deletions test/cli/samples/plugin/advanced/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import {Foo} from "foo";
var foo = new Foo(123);
foo.output();
export {Foo as Bar};
10 changes: 10 additions & 0 deletions test/cli/samples/plugin/advanced/node_modules/foo/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions test/cli/samples/plugin/advanced/node_modules/print/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions test/cli/samples/plugin/advanced/rollup.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
const buble = require('rollup-plugin-buble');

export default {
input: 'main.js',
plugins: [
buble()
],
output: [
{
file: '_actual/cjs.js',
format: 'cjs'
},
{
file: '_actual/es.js',
format: 'esm'
}
]
};
4 changes: 4 additions & 0 deletions test/cli/samples/plugin/basic/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module.exports = {
description: 'basic CLI --plugin functionality',
command: `rollup main.js -f cjs --plugin rollup-plugin-buble`
};
14 changes: 14 additions & 0 deletions test/cli/samples/plugin/basic/_expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use strict';

Object.defineProperty(exports, '__esModule', { value: true });

var Bar = function Bar(x) {
this.x = value;
};
Bar.prototype.value = function value () {
return this.x;
};
var bar = new Bar(123);
console.log(bar.value());

exports.Bar = Bar;
10 changes: 10 additions & 0 deletions test/cli/samples/plugin/basic/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export class Bar {
constructor(x) {
this.x = value;
}
value() {
return this.x;
}
}
var bar = new Bar(123);
console.log(bar.value());
10 changes: 10 additions & 0 deletions test/cli/samples/plugin/cannot-load/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const { assertStderrIncludes } = require('../../../../utils.js');

module.exports = {
description: 'unknown CLI --plugin results in an error',
skipIfWindows: true,
command: `echo "console.log(123);" | rollup --plugin foobar`,
stderr(err) {
assertStderrIncludes(err, "Cannot load plugin 'foobar'");
}
};
10 changes: 10 additions & 0 deletions test/cli/samples/plugin/invalid-argument/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const { assertStderrIncludes } = require('../../../../utils.js');

module.exports = {
description: 'invalid CLI --plugin argument format',
skipIfWindows: true,
command: `echo "console.log(123);" | rollup --plugin 'foo bar'`,
stderr(err) {
assertStderrIncludes(err, 'Invalid --plugin argument format: "foo bar"');
}
};
5 changes: 5 additions & 0 deletions test/cli/samples/plugin/object/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = {
description: 'CLI --plugin object',
skipIfWindows: true,
command: `echo 'console.log(42);' | rollup -f cjs -p '{transform: c => c + String.fromCharCode(10) + c}'`
};
5 changes: 5 additions & 0 deletions test/cli/samples/plugin/object/_expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';

console.log(42);

console.log(42);
5 changes: 5 additions & 0 deletions test/cli/samples/plugin/relative/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = {
description: 'CLI --plugin ../relative/path',
skipIfWindows: true,
command: `echo 'console.log(VALUE);' | rollup -p "../absolute/my-plugin={VALUE: 'relative', ZZZ: 1}"`
};
1 change: 1 addition & 0 deletions test/cli/samples/plugin/relative/_expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log("relative");
2 changes: 1 addition & 1 deletion test/misc/optionList.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.