feat(deps)!: upgrade js-yaml to v5 - #4029
Conversation
js-yaml v5 removes the ESM default export, so `import YAML from 'js-yaml'` now throws at module-load time. Both call sites switch to a namespace import, which keeps the `YAML.load(...)` call sites unchanged: - lib/services/express.js (OpenAPI spec parsing at boot) - modules/tasks/tests/tasks.openapi-operationid.unit.tests.js v5 also switches load() from DEFAULT_SCHEMA to CORE_SCHEMA, dropping merge keys, implicit timestamps, !!binary, !!omap, !!pairs and !!set. The stack's own YAML specs use none of these, so nothing changes stack-side — verified by the operationId test, which parses modules/tasks/doc/tasks.yml for real rather than through a mock. BREAKING CHANGE: downstream projects using `import yaml from 'js-yaml'` must switch to a namespace or named import, and any project YAML relying on merge keys, unquoted dates or !! tags changes meaning under CORE_SCHEMA. See MIGRATIONS.md for the full downstream checklist.
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe project upgrades Changesjs-yaml v5 upgrade
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This upgrades js-yaml and updates the affected imports while preserving existing endpoint gating and parser failure behavior; schema differences and downstream migration steps are documented, and the full unit suite is reported green. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant OpenAPIFile
participant initApiSpec
participant jsYaml
participant Logger
participant Express
initApiSpec->>OpenAPIFile: read raw YAML content
initApiSpec->>jsYaml: load raw YAML
alt content-free document
jsYaml-->>initApiSpec: empty-document error
initApiSpec->>Logger: warn and skip document
else valid document
jsYaml-->>initApiSpec: parsed OpenAPI document
initApiSpec->>Express: register specification route
else malformed document
jsYaml-->>initApiSpec: parsing error
initApiSpec-->>OpenAPIFile: propagate loading failure
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the upgrade, migration changes, schema risks, testing, and downstream impact. It does not follow the repository template and omits the required Scope, Guardrails check, and Notes sections, plus explicit checklist status for lint, tests, and manual checks. Resolution Rewrite the description using the repository template. Add Summary, Scope, Validation, Guardrails check, and Notes sections. Mark each validation and guardrail item, state the impacted modules, cross-module impact, risk level, security considerations, mergeability considerations, and related issue or explicitly state that none applies. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4029 +/- ##
=======================================
Coverage 94.06% 94.07%
=======================================
Files 170 170
Lines 5833 5837 +4
Branches 1867 1869 +2
=======================================
+ Hits 5487 5491 +4
Misses 283 283
Partials 63 63
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Review gate caught two things the v5 migration missed.
1. `load('')` throws in v5 where v4 returned undefined, so an empty (or
whitespace-only) OpenAPI file went from 'skip with a warning' to a rethrown
error — i.e. a boot failure for any downstream project with an empty spec.
express.js now checks the raw content before calling load(), which routes
the empty case back through the existing not-a-plain-object guard. Locked
by a test that fails exactly the way boot would if the guard is removed.
2. MIGRATIONS.md pointed downstream at `{ schema: DEFAULT_SCHEMA }` to restore
YAML 1.1 behaviour, but v5 removed that export. The replacement is
YAML11_SCHEMA. Verified by executing the package, not by reading docs.
Also drops the now-dead `default:` keys from the three js-yaml jest mocks: v5
has no default export, so a mock that supplies one would let a reintroduced
default import pass tests and still fail at boot.
…ion notes
Independent review (CodeRabbit was rate-limited on this public repo) found the
previous commit's guard incomplete and three factual errors in MIGRATIONS.md.
Every claim below was checked by executing js-yaml 5.4.1.
The guard: `raw.trim()` is truthy for a file containing only a comment, but v5
raises 'expected a document, but the input is empty' for that too — as it does
for a BOM-only file. Since config.files.openapi globs modules/*/doc/*.yml, a
downstream module shipping a stubbed or commented-out spec still bricked boot.
express.js now lets load() run and discriminates on err.reason, so all four
content-free shapes skip with a warning while real parse errors still propagate.
The tests: the mocked case is replaced by express.docsEmptySpec.unit.tests.js,
which runs the REAL parser over zero-byte, whitespace-only, comment-only and
BOM-only input, asserts the warning is emitted, and adds two controls — a
malformed document must still fail loudly, a valid one must still be served.
A mock could only ever throw for the inputs its author imagined, which is
exactly how the comment-only case slipped through. Mutation-checked: removing
the guard fails all four cases.
MIGRATIONS.md corrections:
- a '<<:' merge key does NOT raise under CORE_SCHEMA, it is silently kept as a
literal '<<' key. Saying it raises would send a downstream maintainer looking
for a failure that never comes.
- YAML11_SCHEMA is not a drop-in for v4's DEFAULT_SCHEMA. It also restores YAML
1.1 scalar rules v4 did not apply (12:30 -> 750, 014 -> 12, 0o14 -> '0o14',
key y -> true). Documented as a last resort with the divergences listed.
- '!!set' does not load as a native Set under the default schema, it throws
unknown tag; the Set only appears under YAML11_SCHEMA, and serialises to {}.
- the content-free trigger set and the recommended guard now match the code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Follow-up nits from the independent review. A multi-document file is the closest sibling of the guarded condition: v5 raises 'expected a single document in the stream, but found more' from the same function over the same documents array, two lines below the empty-document throw. It is therefore the input any future loosening of the reason check would swallow first, so it earns an explicit control alongside the malformed-syntax one. Also records why loadAll() was not used. It looks like the tidier fix — it returns [] for content-free input and drops the message-string dependency entirely — but it accepts a multi-document file that both v4 and v5 reject, trading a loud failure for a silent partial parse. Drops an HTML entity from the test file's JSDoc that only existed to avoid closing the block comment.
1 participant
What
Upgrades
js-yamlfrom 4.3.2 to 5.4.1 and migrates the two call sites that used the ESM default export.lib/services/express.js:17import YAML from 'js-yaml'->import * as YAML from 'js-yaml'modules/tasks/tests/tasks.openapi-operationid.unit.tests.js:2MIGRATIONS.mdNamespace import rather than named, so every
YAML.load(...)call site stays as it is — this is the migration path js-yaml's own v4->v5 guide recommends for existing code.Why it could not just be a version bump
Dependabot's #3909 proposed the same major as a lockfile-only change and was red. v5 removes the ESM default export, so the old import throws
SyntaxError: The requested module 'js-yaml' does not provide an export named 'default'at module-load time. Sincelib/app.jsimportslib/services/express.js, that breaks application boot, not only the test suite.Schema change — checked, not assumed
v5 switches
load()fromDEFAULT_SCHEMAtoCORE_SCHEMA, which drops merge keys (<<:), implicit timestamps,!!binary,!!omap,!!pairsand!!set;load('')now throws instead of returningundefined.Grepped every
.ymlin the repo for<<:,!!, anchors/aliases and unquotedYYYY-MM-DDscalars: no hits, and no spec file is empty. So nothing changes stack-side. Thetasks.openapi-operationidtest is the real check here — it parsesmodules/tasks/doc/tasks.ymlunmocked, so it exercises the new schema for real.The three
express.docs*tests mockjs-yamland already expose a namedload, so they need no change.Verification
Full unit suite green locally: 171 suites, 2380 tests.
Downstream
MIGRATIONS.mdcarries the checklist: switch default imports, grep project YAML for merge keys / unquoted dates /!!tags, guard any YAML file that can legitimately be empty. MarkedBREAKING CHANGEbecause a downstream project with its own default import breaks on update.Supersedes #3909.
Summary by CodeRabbit
Bug Fixes
Documentation