Default to yaml over js-yaml#859
Conversation
the issue that got it put onto a watch list. 'yaml' is no longer TJ's project but is a completely different repo that is being actively maintained. addresses node-config#851
📝 WalkthroughWalkthroughThe PR replaces the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @parser.js:
- Around line 137-156: Parser.yamlParser currently only logs to console and
returns undefined when neither YAML_DEP nor JS_YAML_DEP are available; change it
to throw a descriptive Error instead. In the lazy-loading block (the require
calls referencing YAML_DEP and JS_YAML_DEP) ensure the final else branch throws
an Error that states neither "yaml" nor "js-yaml" could be loaded and advises
adding one of them to package.json (mentioning both dependency names and that
js-yaml is the fallback), so callers of Parser.yamlParser get a clear exception
instead of undefined.
🧹 Nitpick comments (1)
parser.js (1)
13-15: Nit: keep naming consistent (YamlvsJSYaml).
Yaml(capital Y, lowercase aml) vsJSYamlis a bit asymmetric; considerYAML/JS_YAMLorYaml/JsYamlfor readability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonparser.js
🔇 Additional comments (1)
package.json (1)
40-41: No action needed—YAML placement is intentional per project design.All format parsers (yaml, toml, cson, hjson, properties, x2js, coffeescript) are in devDependencies as soft dependencies. This is consistent with the project's stated approach ("Define soft dependencies so transpilers don't include everything"). The code already handles missing parsers gracefully: if neither
yamlnorjs-yamlis installed, it logs a clear error message suggesting the user add the dependency. The [email protected] package fully supports CommonJS require() and Node.js 20.
markstos
left a comment
markstos
left a comment
There was a problem hiding this comment.
QA Log
- Reviewed related issue and diff.
2 participants
js-yaml is mostly dead although it did get a CVE patch to address the issue that got it put onto a watch list.
'yaml' is no longer TJ's project but is a completely different repo that is being actively maintained.
addresses #851
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.