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

Additional Configuration File Support #518

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcknasty
Copy link
Contributor

@mcknasty mcknasty commented Feb 2, 2024

#Temp PR Message

feat: Adding additional configuration file options
test: unit test for config file detection
test: unit test for config files pass via the cli
test: unit test case for early exit of hideInstrumenteeArgs
test: unit test for Node.js argument handling
fix: bug in spawning of c8 process that dropped coverage significantly

Checklist
  • npm test, tests passing
  • npm run test:snap (to update the snapshot)
  • tests and/or benchmarks are included
  • documentation is changed or added

@mcknasty mcknasty changed the title Additional Configuration File Support (#436) Additional Configuration File Support Feb 2, 2024
@mcknasty
Copy link
Contributor Author

mcknasty commented Feb 2, 2024

Reference to old PR (#436)

@mcknasty mcknasty marked this pull request as draft February 2, 2024 19:13
@bcoe
Copy link
Owner

bcoe commented Feb 19, 2024

I'm hesitant to iuntroduce js-yaml dependency which is fairly large and doesn't seem to be updated frequently. Also, historically yaml is a complex enough DSL that it is known to have serious security vulnerabilities in some implementations.

A couple options:

  • Could we allow users to bring their own copy of js-yaml and print a warning if they try to use yaml configuration without installing the dependency (to use yaml configuration please install js-yaml as a dev dependency) do any other tools do something like this?
  • Long term solution, a yaml parser seems like something worth considering in Node core.

@mcknasty
Copy link
Contributor Author

mcknasty commented Feb 23, 2024

My regrets, this pull request is still in draft mode and not completely up to date with my local fork. I will get this branch updated during my next programming cycle. @kf6kjg has been helping out with the latest version of this branch.

To partially address your concern on the js-yaml package, it's already included in the in the node_module directory. This is due to the fact it's a dependency of eslint which is a dependency of standard, which is, of course, a dev-dependency of this project.

I do understand the serious security vulnerabilities of certain implementations and I agree with that concern. I like your byo yaml parser option, though we might need to rethink using standard for linting. It does have the js-yaml sub-dependency. Oddly enough, if I don't include it in the dependency section of the package.json file, it can still be loaded as a requirement in the implementation.

Any thoughts about extending the configuration section to include 3rd party yaml cli tools? The cli tool yq looks promising.

Let me do some additional research on this topic and see what I find out.

@kf6kjg
Copy link

kf6kjg commented Feb 23, 2024

By moving JS-yaml to being lazy loaded we could make it an optional peer dependency. Then if the user needs it they can install it. Everyone else doesn't get it in their tree.

@bcoe
Copy link
Owner

bcoe commented Jun 10, 2024

(@mcknasty let's keep this PR open and drive it to completion, thank you for your contributions)

@kf6kjg @mcknasty, I'm very much on the fence with yaml. One one hand, I understand users would love to be able to use yaml. On the other hand, I spend most of my OSS time updating dependencies to deal with transitive security issues, and the fact that the yaml libraries are a fairly large surface makes me consider them a good potential candidate to get CVEs logged against them.

Lazy loading is perhaps a good approach, do you think users would be willing to add js-yaml as a dev dependency if we throw an error that explains how?

@kf6kjg
Copy link

kf6kjg commented Jun 10, 2024

IIRC those were the goals.

@bcoe bcoe mentioned this pull request Jun 11, 2024
4 tasks
@mcknasty mcknasty mentioned this pull request Jun 12, 2024
12 tasks
@mcknasty
Copy link
Contributor Author

mcknasty commented Jun 14, 2024

I would like to request some clarity on the comments above. We don't need to explicitly include js-yaml as a development dependency. The unit tests pass for this commit , if I remove js-yaml from package.json. The package is still included in the node_module directory due to the fact it's a dependency of eslint. Furthermore, eslint is a dependency of standard.

@bencoe Are you planning on moving away from standard as a lint tester? Is that why we want to prompt for an optional download or path for a yaml parsing binary?

Additionally, this pull request has a dependency on #517. The print derived config option is required to verify what type of file has been loaded by yargs in the pending unit tests.

@bcoe, #517 was at 100% coverage and I am fairly certain it was passing through the CD/CI. There were only two concerns we were working through. Would you like me to open a new pull request? There was a comment about test setup functions. Is there code style guide you could refer me to?

If there is anything additional you would like to discuss, please feel free to email me at the email address in the commit log.

@kf6kjg
Copy link

kf6kjg commented Jun 14, 2024

IMO being a transitive dependency is fine. Ifnwhen those packages decide to remove js-yaml, or even make it an optional peer dependency like I suggested, then it could disappear entirely. In the mean time this package only declares that if you need it you can install and use it. In my case I probably wont, I want the CJS configs these days! :D

feat: Adding additional configuration file options
test: unit test for config file detection
test: unit test for config files pass via the cli
test: unit test case for early exit of hideInstrumenteeArgs
test: unit test for Node.js argument handling
fix: bug in spawning of c8 process that dropped coverage significantly
@mcknasty
Copy link
Contributor Author

js-yaml doesn't require c8. Is there a reason it needs to be a peer dependency? Can it just be an optional dependency?

@kf6kjg
Copy link

kf6kjg commented Jun 14, 2024

Thank you for pointing out optional dependencies, that one I'd not directly worked with before so it was good to learn.

From what I learnt: Optional dependencies mean that if it is omitted via the --omit=optional NPM flag it will not be installed, but if it is then if there is a conflicting versions specified by the root package or some other package then c8 will get its own copy.

Additionally peer dependencies are more for "plugin" architectures, which in this case c8 isn't doing: we aren't passing to c8 an instance of js-yaml. Thus I think that you are correct in pointing out that being an optional dependency is the more correction option.

@bcoe
Copy link
Owner

bcoe commented Jun 26, 2024

@mcknasty @kf6kjg why don't we compromise here, and instead of adding yaml support add toml support with:

https://github.com/squirrelchat/smol-toml#readme

I see smol-toml being used by the project Redwood, which was created by TOML's creator, and it seems to be a pretty small dependency that's actively maintained.

@kf6kjg
Copy link

kf6kjg commented Jun 26, 2024

I love TOML, used it myself one a long time back, but it's never gotten the traction that YAML has and it would be a lot harder on the users to figure out. That said, while TOML solves #420, I've come to the conclusion that I'm more a fan of #425's JS-based, and TS-based in the future, configs which as a side effect ALSO solves the basic want of #420 - comments. Thus if YAML is full blocker for you, though I'm still not sure why, and @mcknasty doesn't feel like implementing TOML then I'd say we cull the YAML support and get the ball rolling on the JS support.

@bcoe
Copy link
Owner

bcoe commented Jun 26, 2024

Thus if YAML is full blocker for you, though I'm still not sure why, and @mcknasty doesn't feel like implementing TOML then I'd say we cull the YAML support and get the ball rolling on the JS support.

This sounds like a great compromise to me, I like that .js and .mjs support wouldn't require us pulling in a beefy library. And it should address many of the same requirements.

Sorry for being such a pain in the neck on this PR. I've grown over the years to be much more conservative with dependency trees.

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