Skip to content

Optionally pass CLI command arguments to config - #1926

Merged
lukastaegert merged 1 commit into
rollup:masterfrom
borilla:config-function
Mar 15, 2018
Merged

Optionally pass CLI command arguments to config#1926
lukastaegert merged 1 commit into
rollup:masterfrom
borilla:config-function

Conversation

@borilla

@borilla borilla commented Jan 29, 2018

Copy link
Copy Markdown
Contributor

Please see #1925 for a discussion relating to this pull request

If config file exports a function then call this function with commandOptions object. We expect this function to return a config object or configs array, the same as existing config files

If config file exports a function then call this function with `commandOptions` object. We expect this function to return a `config` object or `configs` array, the same as existing config files
@borilla borilla changed the title Optionally pass CLI command arguments to config #1925 Optionally pass CLI command arguments to config Jan 29, 2018

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for not having a look at this earlier. I actually like the idea! It also gives you the chance to add "custom" CLI options to your build without the need to modify anything on rollup's side (as long as there is no conflict with rollup's own options). For instance I could imagine people using npx rollup -c --production to trigger a production build etc.

@lukastaegert lukastaegert added this to the 0.57.0 milestone Mar 14, 2018
@lukastaegert

Copy link
Copy Markdown
Member

@guybedford What do you think?

@lukastaegert
lukastaegert merged commit cd77bfb into rollup:master Mar 15, 2018
@guybedford

guybedford commented Mar 15, 2018

Copy link
Copy Markdown
Contributor

@lukastaegert sorry I missed this one. I haven’t looked closely, but does this stop us from verifying invalid CLI arguments or misspellings like --formt instead of —format? Just checking as that would be a shame to lose here.

@lukastaegert

Copy link
Copy Markdown
Member

As a matter of fact, misspelled CLI options never seemed to have been verified in the first place. So there is no regression here which makes this feature much more interesting. If we wanted to introduce such a check, however, we should consider interactions with this feature now. E.g. maybe the config file should be responsible for cleaning up the command object.

@guybedford

Copy link
Copy Markdown
Contributor

Yeah that was just my concern if we're closing a door here at all.

@guybedford

Copy link
Copy Markdown
Contributor

The other worry is that if there becomes a common flag in use by a very popular plugin, then Rollup cannot use that flag itself as a value due to the namespace collision.

Could we not have a special argument for this like using the -- argument or something:

rollup x.js --format amd -- asdf custom args

The more I think about this I don't think we should release this in the current form.

@lukastaegert

Copy link
Copy Markdown
Member

The other worry is that if there becomes a common flag in use by a very popular plugin, then Rollup cannot use that flag itself as a value due to the namespace collision.

This is not an issue as this is not about plugins at all, this is solely about local config files. Plugins could not and still cannot consume CLI options.

One primary use case for config files is that you can inspect rollup's options to react e.g. to the use of --silent etc. One side effect is that you can now define your own options and I do not see any issue with that as people usually write their config files themselves.

@lukastaegert

Copy link
Copy Markdown
Member

We could however reserve a namespace for custom variables i.e. that they should be of the form --custom-myarg. Then we could display a warning for all other unknown arguments but allow and ignore all arguments starting with custom.

@guybedford

Copy link
Copy Markdown
Contributor

This is not an issue as this is not about plugins at all, this is solely about local config files. Plugins could not and still cannot consume CLI options.

Right, the same concern applies to configuration files though in that a new rollup flag could break existing configurations.

So I do think we need some way to disambiguate the namespaces.

--config-x or --config-flags --x --y asdf (where all arguments after --config-flags are taken out early in the processing) both work for me.

@lukastaegert

Copy link
Copy Markdown
Member

I think config-x would be easier to implement so I see what can be done.

@guybedford

Copy link
Copy Markdown
Contributor

Sounds good!

@borilla

borilla commented Mar 15, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for the positive discussion

Right, the same concern applies to configuration files though in that a new rollup flag could break existing configurations.

I agree this is the major potential problem with this change but, personally, I'd say that plugin authors could be left (or perhaps advised) to handle this concern themselves by providing non-clashing names, eg by prefixing plugin-specific params with the plugin's name: --plugin-name-silent

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.

3 participants