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

Consider switching YAML libraries #13

Open
eemeli opened this issue Apr 19, 2020 · 1 comment
Open

Consider switching YAML libraries #13

eemeli opened this issue Apr 19, 2020 · 1 comment

Comments

@eemeli
Copy link

eemeli commented Apr 19, 2020

At the moment, YAML files are loaded with require('js-yaml').load(), while JSON uses JSON.parse(). It might make sense to switch both of these to use require('yaml').parse() instead. This would have the following benefits:

  • yaml has better YAML spec compliance; see the YAML Test Matrix, which shows results for js-yaml as "js-yaml-json" and yaml as "js-event" and "js-json".
  • Safer config data loading, as the js-yaml load() supports JavaScript being encoded in YAML files. Obviously NYC also supports .js files for config, but it might still be surprising for a config loaded from a YAML file not to be guaranteed inert.
  • Better error reporting, in particular for JSON files -- JSON is a subset of YAML, and can be loaded as such.
  • Support for # comments in JSON files.

As disclosure, I'm the developer of yaml, so my opinions may be a little biased here. I'm interested, as yaml itself ends up having @istanbuljs/load-nyc-config as a dev dependency. I would be quite willing to submit a PR for this change, but thought it'd be more polite to ask first.

@coreyfarrell
Copy link
Member

Part of the decision when .nycrc.yml support was initially added was the ability for npm to install a yaml parser that would be shared with multiple development tools, in particular eslint and mocha both use js-yaml. Have you approached those projects with this same proposal?

Looking at the yaml package one concern I have is the inclusion of browser support, this significantly increases the install size. At quick glance of the browser code I'm confused about it using ES module syntax yet pulling in @babel/runtime. This seems like a contradiction, if a browser supports ES modules then it shouldn't need @babel/runtime?

Obviously size is not the only consideration but it does exist. Additionally although I wouldn't expect user-facing issues from switching yaml processors I would still mark such a change as 'breaking' just in case (the encoded JS as an example).

Please don't be discouraged by my response, I'm not against this idea just don't want to make such a change without scrutinizing.

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

No branches or pull requests

2 participants