Repository navigation
lib: add experimental node:yaml module - #67157
marcopiraccini wants to merge 1 commit into
Conversation
|
Review requested:
|
405299b to
0d87e9f
Compare
ad1f666 to
09ed139
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #67157 +/- ##
==========================================
+ Coverage 90.69% 90.73% +0.03%
==========================================
Files 797 799 +2
Lines 278787 280456 +1669
Branches 53607 54257 +650
==========================================
+ Hits 252858 254467 +1609
- Misses 16292 16343 +51
- Partials 9637 9646 +9
🚀 New features to boost your workflow:
|
Add a YAML 1.2 parser for the core schema as `node:yaml`, behind `--experimental-yaml`, with `yaml.parse(input)` following the shape of `Bun.YAML.parse()`. The parser is written from scratch, in one pass from text to values, with no new dependency. Anchors and aliases resolve to shared objects, core schema tags are applied, mappings become plain objects and duplicate keys are an error. The official yaml-test-suite is vendored as a JSON fixture and run by test-yaml-test-suite.js: 400 of its 402 cases pass, the two left are documents with duplicate keys. Refs: nodejs#53787 Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>
09ed139 to
bbfacd1
Compare
LiviaMedeiros
left a comment
There was a problem hiding this comment.
I'm +0.5 for adding builtin yaml support in general and would be +1 if some concerns were resolved first:
- How do we do versioning once
v1.3or subsequent specs are released? While JSON is both blessed in cursed with lack of versioning, in YAML we need to account this somehow. Hardcodingv1.2.2isn't very future-proof.
1.1. Should we introduce compatibility withv1.1? - What's the plan for this module, aside from
parse()andstringify()? Are we aiming for full-fledged module with all the classes, traversal methods, selectors, transformers, handling multi-document objects, etc.?
Without draft for the desired module API, it's hard to tell if it really should be implemented as separate builtin module.
If we just need this for something similar to #53787, i think an internal/util.parseYAML() (potentially re-exported later for userland as util.parseYAML() or util.YAML.parse()) would be sufficient.
| // parser. test/fixtures/yaml-test-suite.json holds, for every test, the input | ||
| // and either the expected documents as JSON or a flag that the input is an | ||
| // error. |
There was a problem hiding this comment.
Nit: would be more future-proof if either the file structure was fully defined here, or if there was a tool that regenerates it if test suite gets updated.
| let TextDecoder; | ||
| function decode(input) { | ||
| TextDecoder ??= require('internal/encoding').TextDecoder; | ||
| return new TextDecoder('utf-8').decode(input); | ||
| } |
There was a problem hiding this comment.
Nit
| let TextDecoder; | |
| function decode(input) { | |
| TextDecoder ??= require('internal/encoding').TextDecoder; | |
| return new TextDecoder('utf-8').decode(input); | |
| } | |
| let textDecoder; | |
| function decode(input) { | |
| textDecoder ??= new (require('internal/encoding').TextDecoder)('utf-8'); | |
| return textDecoder.decode(input); | |
| } |
ShogunPanda
left a comment
There was a problem hiding this comment.
I'm +1 to this. Actually I wanted to implement it but Marco beated me on time.
Only a small nit: make it clear that now it only adds the parser in the commit.
|
Can we verify if a native version would be faster? I'm +1 on adding it to core, but I think we should strive to have near-json performance if possible. |
Why was this path chosen over forking a known tested module and vendoring it? |
| if (documents.length === 0) return null; | ||
| if (documents.length === 1) return documents[0]; | ||
| return documents; | ||
| } |
There was a problem hiding this comment.
A non-throwing parse variant (returning null on failure to parse) would be helpful to avoid allocating the error stack in cases it's not needed
|
Should we pick TOML instead? It's much easier to parse (and a simpler parser usually means less maintenance burden), even for humans arguably |
It shouldn't be "instead." There are many reasons to parse yaml, and the standard library is not currently providing a means to do so. |
This is a proposal for a built-in YAML parser, with a basic implementation to discuss on: an experimental, parse-only
node:yamlmodule behind--experimental-yaml.The parser supports the YAML 1.2 core schema, anchors, aliases, tags, and multi-document streams. It is written from scratch and has no external dependencies.
This gives Node.js a built-in YAML parser for future configuration support, following the discussion about
.nodercin #53787, where the config file ended up JSON-only because core had no parser.Bun parity
Bun ships
Bun.YAMLsince v1.2.21.yaml.parse()followsBun.YAML.parse()so that code written for it runs unchanged:parse(input)takes a string or binary input (ArrayBuffer,TypedArray,DataView) decoded as UTF-8.null.SyntaxError(here with theERR_INVALID_YAMLcode and the line and column).!!strand!!intare applied.Not covered yet:
Bun.YAML.stringify(value, replacer, space),Blobinput, and importing.yamlfiles as modules. See the next steps.Tests
Tests include the official YAML Test Suite, vendored at a fixed revision. Of 402 cases, 400 pass.
The two that fail, 2JQS and X38W, are documents with duplicate keys: two empty keys, and a key that is an alias of the previous key. The suite only checks that they parse, since duplicate keys are not a syntax error, and provides no expected output for them. The YAML 1.2 specification requires mapping keys to be unique, and this parser enforces it and throws
Duplicate mapping key, as theyamlandjs-yamlpackages do by default. They are listed as known failures with that reason. If the reviewers prefer theJSON.parse()behavior, where the last value wins, the check can be removed and both pass.Review guide
doc/api/yaml.md: API behavior.lib/yaml.js: input validation and document return values.lib/internal/yaml/parser.js: parser implementation.test/parallel/test-yaml.js: API tests.test/parallel/test-yaml-test-suite.js: official suite runner.node:vfssetup in vfs: add minimal node:vfs subsystem #63115.Next steps, if accepted
yaml.stringify(value, replacer, space)with Bun's semantics.import data from './config.yaml' with { type: 'yaml' }.--experimental-config-file.yamlsubsystem label and commit subsystem, as was done forvfs.Refs: