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

introduce CLI --plugin support #3379

merged 7 commits into from Feb 28, 2020

Conversation

kzc
Copy link
Contributor

@kzc kzc commented Feb 10, 2020

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:
#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
Copy link

codecov bot commented Feb 10, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

kzc commented Feb 10, 2020

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
Copy link
Contributor Author

kzc commented Feb 22, 2020

Added a few tests to pass code coverage.

@kzc
Copy link
Contributor Author

kzc commented Feb 22, 2020

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
Copy link
Contributor Author

kzc commented Feb 23, 2020

Tested CLI plugin with absolute path:

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

Code coverage is passing once again.

kzc and others added 2 commits Feb 23, 2020
- 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}"
Copy link
Member

@lukastaegert lukastaegert left a comment

Thanks for adding the tests. Mostly questions for now.

cli/run/index.ts Outdated
let pluginArg : any = undefined;
if (pluginText[0] === '{') {
// -p "{transform(c,i){...}}"
plugin = new Function('return ' + pluginText);
Copy link
Member

@lukastaegert lukastaegert Feb 25, 2020

Choose a reason for hiding this comment

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

Nice one!

cli/run/index.ts Outdated
}
if (!plugin) {
try {
if (pluginText[0] == ".") pluginText = process.cwd() + "/" + pluginText;
Copy link
Member

@lukastaegert lukastaegert Feb 25, 2020

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

@kzc kzc Feb 25, 2020

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.

cli/run/index.ts Outdated
} else {
throw new Error(`Invalid --plugin argument format: ${JSON.stringify(pluginText)}`);
}
if (!/^\.|[@\/\\]/.test(pluginText)) {
Copy link
Member

@lukastaegert lukastaegert Feb 25, 2020

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

@kzc kzc Feb 25, 2020

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

@kzc kzc Feb 27, 2020

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

@lukastaegert lukastaegert Feb 27, 2020

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 🙄

cli/run/index.ts Outdated
// 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

@lukastaegert lukastaegert Feb 25, 2020

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

@kzc kzc Feb 25, 2020

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.

@kzc
Copy link
Contributor Author

kzc commented Feb 25, 2020

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
Copy link
Member

lukastaegert commented Feb 27, 2020

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!

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}}"`
Copy link
Contributor Author

@kzc kzc Feb 27, 2020

Choose a reason for hiding this comment

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

typo - stray trailing backtick

@kzc
Copy link
Contributor Author

kzc commented Feb 27, 2020

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.

- Via an inline implementation:

```
rollup -i input.js -f es -p '{transform: c => "/* TEST */" + c}
Copy link
Contributor Author

@kzc kzc Feb 27, 2020

Choose a reason for hiding this comment

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

typo - need a trailing single quote

Copy link
Contributor Author

@kzc kzc Feb 27, 2020

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@kzc kzc Feb 27, 2020

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@kzc kzc Feb 27, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@lukastaegert lukastaegert Feb 28, 2020

Choose a reason for hiding this comment

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

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 👍

Copy link
Member

@lukastaegert lukastaegert Feb 28, 2020

Choose a reason for hiding this comment

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

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...

Copy link
Member

@lukastaegert lukastaegert left a comment

Will ship it now

@lukastaegert lukastaegert merged commit 07223eb into rollup:master Feb 28, 2020
7 checks passed
@kzc kzc deleted the cli--plugin branch Feb 28, 2020
@fregante
Copy link
Member

fregante commented Feb 28, 2020

Woot! Thanks for fixing #1304

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

3 participants