introduce CLI --plugin support #3379

kzc · 2020-02-10T09:11:29Z

This PR contains:

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:
#1304

Description

Implement CLI --plugin support

provide plugin prefixes as required
support new and old styles of plugin naming

Examples:

--plugin rollup-plugin-buble
--plugin @rollup/plugin-buble
--plugin buble
-p "@rollup/plugin-replace={DBG:true}"
-p node-resolve,commonjs -p "terser={output:{beautify:true}}"
-p "/absolute/path/to/plugin={A:1}"
-p "../relative/path/to/plugin={B:2}"

codecov · 2020-02-10T09:18:57Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a179256). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3379   +/-   ##
=========================================
  Coverage          ?   93.29%           
=========================================
  Files             ?      172           
  Lines             ?     6112           
  Branches          ?     1822           
=========================================
  Hits              ?     5702           
  Misses            ?      218           
  Partials          ?      192

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a179256...dbd2f3d. Read the comment docs.

kzc · 2020-02-10T09:29:59Z

Example:

$ cat input.js 
import fn from "is-plain-object";
[1, "2", console, {A: 3}, [4]].forEach(x => console.log(JSON.stringify(x), fn(x)));

without CLI specified plugins:

$ dist/bin/rollup input.js --silent
import fn from 'is-plain-object';

[1, "2", console, {A: 3}, [4]].forEach(x => console.log(JSON.stringify(x), fn(x)));

with CLI specified plugins:

$ dist/bin/rollup input.js -p node-resolve,commonjs -p "terser={output:{comments:false}}" --silent
function o(o){return!0==(null!=(t=o)&&"object"==typeof t&&!1===Array.isArray(t))&&"[object Object]"===Object.prototype.toString.call(o);var t}[1,"2",console,{A:3},[4]].forEach(t=>{return console.log(JSON.stringify(t),!1!==o(r=t)&&"function"==typeof(e=r.constructor)&&!1!==o(n=e.prototype)&&!1!==n.hasOwnProperty("isPrototypeOf"));var r,e,n});

$ dist/bin/rollup input.js -p node-resolve,commonjs -p "terser={output:{comments:false}}" --silent | node
1 false
"2" false
{} false
{"A":3} true
[4] false

The tests pass on all platforms. Code coverage is failing, but I don't have much patience for that sort of thing.

@lukastaegert Please feel free to make changes to this PR. I don't plan to work on it further.

kzc · 2020-02-22T05:41:05Z

Added a few tests to pass code coverage.

kzc · 2020-02-22T18:10:58Z

Now supports new and old styles of plugin naming. Will add prefix if needed.

Examples:

--plugin rollup-plugin-buble
--plugin @rollup/plugin-buble
--plugin buble
-p "@rollup/plugin-replace={DBG:true}"
-p node-resolve,commonjs -p "terser={output:{beautify:true}}"

Unfortunately code coverage fails again. Will add a test when I get a chance.

kzc · 2020-02-23T04:58:43Z

Tested CLI plugin with absolute path:

    -p "/absolute/path/to/my-plugin={ABC:123}"

Code coverage is passing once again.

- provide plugin prefixes as required - support new and old styles of plugin naming Examples: --plugin rollup-plugin-buble --plugin @rollup/plugin-buble --plugin buble -p "@rollup/plugin-replace={DBG:true}" -p node-resolve,commonjs -p "terser={output:{beautify:true}}" -p "/absolute/path/to/plugin={A:1}" -p "../relative/path/to/plugin={B:2}"

lukastaegert

Thanks for adding the tests. Mostly questions for now.

lukastaegert · 2020-02-25T05:48:10Z

cli/run/index.ts

+		let pluginArg : any = undefined;
+		if (pluginText[0] === '{') {
+			// -p "{transform(c,i){...}}"
+			plugin = new Function('return ' + pluginText);


lukastaegert · 2020-02-25T05:59:22Z

cli/run/index.ts

+			}
+			if (!plugin) {
+				try {
+					if (pluginText[0] == ".") pluginText = process.cwd() + "/" + pluginText;


Why not just path.resolve(pluginText)?

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.

lukastaegert · 2020-02-25T06:02:55Z

cli/run/index.ts

+			} else {
+				throw new Error(`Invalid --plugin argument format: ${JSON.stringify(pluginText)}`);
+			}
+			if (!/^\.|[@\/\\]/.test(pluginText)) {


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('.'))...?

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.

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.

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

lukastaegert · 2020-02-25T06:08:48Z

cli/run/index.ts

+				// 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)) {


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.

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.

kzc · 2020-02-25T14:10:23Z

The tests were ten times more work than implementing the feature itself. But it did bring to light that relative plugins were not working, which was subsequently corrected.

Feel free to reimplement the feature through other means as long as all the tests pass. I think this PR adds much needed usability to the CLI. WIth stdin support and this PR, I don't have much need for rollup config files for most tasks now.

lukastaegert · 2020-02-27T06:50:34Z

The tests were ten times more work than implementing the feature itself. But it did bring to light that relative plugins were not working, which was subsequently corrected

I know it is often the main part of the work, thanks for writing them ❤️ And not only did they bring an issue to light already, they also helped me very much writing the documentation and understanding all the cases you were considering. Also, they will make future refactorings safe.

Besides adding documentation, I found that errors were resulting in partly swallowed "unhandled promise rejections" which I fixed so that they are displayed properly just as any other error. From my side, this is ready for release now if you do not have anything more to add, thanks again for the work, this IS a great feature!

kzc · 2020-02-27T08:10:08Z

docs/01-command-line-reference.md

+By default, plugins that export functions will be called with no argument to create the plugin. You can however pass a custom argument as well:
+
+```
+rollup -i input.js -f es -p "terser={output: {beautify: true, indent_level: 2}}"`


typo - stray trailing backtick

kzc · 2020-02-27T08:23:37Z

Nice work on the docs and proper error handling - I wasn't sure how you wanted that handled. I also learned what path.resolve does.

LGTM - ship it.

kzc · 2020-02-27T09:00:22Z

docs/01-command-line-reference.md

+- Via an inline implementation:
+
+  ```
+  rollup -i input.js -f es -p '{transform: c => "/* TEST */" + c}


typo - need a trailing single quote

Looks like the example doesn't work unless you add a newline - rollup drops the comment otherwise:

-p '{transform: c => "/* TEST */\n" + c}'

This example is a bit more interesting - it's up to you:

-p '{transform: (c, i) => `/* ${i} */\n` + c}'

Or perhaps:

-p '{transform: (c, i) => `/* ${JSON.stringify(i)} */\n` + c}'

which would show embedded \0 (nils) for other plugins.

Try running the following and then again without the newline after the comment:

dist/bin/rollup test/cli/samples/plugin/advanced/main.js -p node-resolve,commonjs -p '{transform: (c, i) => `/* ${JSON.stringify(i)} */\n` + c}' --silent

Nice catch. Without the newline, Rollup only keeps the comment if the first line is preserved, which was the case for my test call 😉 With the newline, it is always preserved as it is considered a banner.

I will change to using your example 👍

Fixed. I also consistenly used single quotes around both examples because otherwise, Bash might do variable expansions or let hell break loose when you use backticks...

lukastaegert

Will ship it now

fregante · 2020-02-28T11:33:46Z

Woot! Thanks for fixing #1304

kzc and others added 2 commits February 23, 2020 12:20

Merge branch 'master' into cli--plugin

5efed08

lukastaegert reviewed Feb 25, 2020

View reviewed changes

lukastaegert and others added 2 commits February 27, 2020 07:46

Add documentation, fix error handling

a179256

Merge branch 'master' into cli--plugin

b3543a3

Fix markdown formatting

d6d94da

kzc commented Feb 27, 2020

View reviewed changes

Remove trailing slash

721beb8

kzc commented Feb 27, 2020

View reviewed changes

Fix examples in documentation

dbd2f3d

lukastaegert approved these changes Feb 28, 2020

View reviewed changes

lukastaegert merged commit 07223eb into rollup:master Feb 28, 2020

kzc deleted the cli--plugin branch February 28, 2020 05:55

This was referenced Mar 7, 2020

[Snyk] Upgrade rollup from 0.63.5 to 1.32.0 tjenkinson/DefinitelyTyped#4

Open

[Snyk] Upgrade rollup from 1.27.2 to 1.32.0 tjenkinson/rollup-bug-empty-dymanic-import#2

Open

[Snyk] Upgrade rollup from 0.57.1 to 1.32.0 tjenkinson/www.jsdelivr.com#3

Open

This was referenced Mar 22, 2020

[Snyk] Upgrade rollup from 1.31.1 to 1.32.0 ui5-next/gulp-ui5-eager-preload#19

Merged

[Snyk] Upgrade rollup from 1.31.1 to 1.32.0 Tootoot222/react-typescript-hook-mouse#20

Closed

[Snyk] Upgrade rollup from 1.29.0 to 1.32.0 jamesleesaunders/d3-x3d#148

Merged

snyk-bot mentioned this pull request Mar 29, 2020

[Snyk] Upgrade rollup from 1.32.0 to 2.0.0 ui5-next/gulp-ui5-eager-preload#21

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce CLI --plugin support #3379

introduce CLI --plugin support #3379

kzc commented Feb 10, 2020 •

edited

Loading

codecov bot commented Feb 10, 2020 •

edited

Loading

kzc commented Feb 10, 2020 •

edited

Loading

kzc commented Feb 22, 2020

kzc commented Feb 22, 2020

kzc commented Feb 23, 2020

lukastaegert left a comment

lukastaegert Feb 25, 2020

lukastaegert Feb 25, 2020

kzc Feb 25, 2020

lukastaegert Feb 25, 2020

kzc Feb 25, 2020

kzc Feb 27, 2020

lukastaegert Feb 27, 2020

lukastaegert Feb 25, 2020

kzc Feb 25, 2020

kzc commented Feb 25, 2020

lukastaegert commented Feb 27, 2020

kzc Feb 27, 2020

kzc commented Feb 27, 2020

kzc Feb 27, 2020

kzc Feb 27, 2020

kzc Feb 27, 2020

kzc Feb 27, 2020 •

edited

Loading

lukastaegert Feb 28, 2020

lukastaegert Feb 28, 2020

lukastaegert left a comment

fregante commented Feb 28, 2020

introduce CLI --plugin support #3379

introduce CLI --plugin support #3379

Conversation

kzc commented Feb 10, 2020 • edited Loading

Description

codecov bot commented Feb 10, 2020 • edited Loading

Codecov Report

kzc commented Feb 10, 2020 • edited Loading

kzc commented Feb 22, 2020

kzc commented Feb 22, 2020

kzc commented Feb 23, 2020

lukastaegert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Feb 25, 2020

lukastaegert commented Feb 27, 2020

Choose a reason for hiding this comment

kzc commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert left a comment

Choose a reason for hiding this comment

fregante commented Feb 28, 2020

kzc commented Feb 10, 2020 •

edited

Loading

codecov bot commented Feb 10, 2020 •

edited

Loading

kzc commented Feb 10, 2020 •

edited

Loading

kzc Feb 27, 2020 •

edited

Loading