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

Split config extends functionality to separate module? #1374

Open
coreyfarrell opened this issue Jul 16, 2019 · 4 comments
Open

Split config extends functionality to separate module? #1374

coreyfarrell opened this issue Jul 16, 2019 · 4 comments

Comments

@coreyfarrell
Copy link
Contributor

Ref istanbuljs/load-nyc-config#2

The module in question is to be used by both nyc and babel-plugin-istanbul. The babel plugin does not use yargs (no CLI processing), but we want it to load the same configuration as nyc. Currently extends is not processed. I could probably use const applyExtends = require('yargs/lib/apply-extends') but this is not documented as a public API and the babel plugin has no reason to install yargs as a whole. It would be nice if lib/apply-extends.js were it's own stand-alone module so the babel plugin could get identical config loading functionality as nyc without pulling in all of yargs.

@bcoe
Copy link
Member

bcoe commented Jul 16, 2019

well we're at it we should address this bug: #1372 😝

I'm a little torn, because I've tried to be pretty conservative about splitting yargs into packages:

  • yargs has more dependencies than many of the other popular CLI frameworks floating around (and has been criticized for this).
  • it adds extra maintenance burden to manage additional modules.

If we think that extends' logic is valuable enough to extract, perhaps we can just continue vendoring it inside yargs, but extract it into its own module; then just add a note in the file that it shouldn't be edited in place.

@coreyfarrell
Copy link
Contributor Author

I was thinking to avoid pulling all of yargs into babel-plugin-istanbul but realistically any user of this babel plugin likely uses nyc as well or some other development module that uses yargs. As I mentioned I can get the function I need by using require('yargs/lib/apply-extends'). Normally breaking changes to an internal API would not spawn a semver-major release. If we can treat this as a public API entry-point (breaking change to it is semver-major), then I will be happy enough. Even if it's not a publicly documented API, just a comment in lib/apply-extends.js saying that the function is a public API.

@bcoe
Copy link
Member

bcoe commented Jul 16, 2019

@coreyfarrell what I don't love about this is, you do end up pulling in the 224kb yargs package, which folks are bound to call attention to 😛

Let's split it into a module? and I'll just vendor it.

@coreyfarrell
Copy link
Contributor Author

Actually I'm planning to vendor it into @istanbuljs/load-nyc-config, this way I can do things more specific to nyc - for example support "extends": "./something.yml".

See istanbuljs/load-nyc-config#4

Leaving this ticket open for now so we can decide if some of the enhancements I made should be contributed back to yargs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants