feat: add (de)serialization options with a validation opt-out - #312
Merged
JBBianchi merged 2 commits intoAug 20, 2026
Conversation
Signed-off-by: Jean-Baptiste Bianchi <jb.bianchi@neuroglia.io>
There was a problem hiding this comment.
Pull request overview
This PR evolves the Workflow (de)serialization API to use an options payload (instead of accumulating positional booleans) and adds a validation opt-out to support editor/work-in-progress workflows. It also fixes ArrayHydrator coercion bugs that become reachable when validation is skipped, and updates tests/docs/examples accordingly.
Changes:
- Add
SerializationOptions/DeserializationOptions(plus YAML-specific output options) and support validation opt-out inserialize()anddeserialize(), while keeping the legacy positional signature (deprecated). - Fix
ArrayHydratorconstructor behavior for empty/null/non-array models and large arrays. - Expand test coverage and update README + examples to the options-based API.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/4_generate-classes.ts | Updates codegen for Workflow overloads/options and validation opt-out behavior. |
| src/lib/generated/classes/workflow.ts | Regenerated Workflow class to implement options payload, validation opt-out, YAML dump options. |
| src/lib/serialization.ts | New shared public option types and helpers for normalizing serialize args / YAML dump options. |
| src/open-workflow-sdk.ts | Re-exports serialization-related option types from the package entry point. |
| src/lib/hydrator.ts | Fixes ArrayHydrator constructor to avoid numeric-coercion pitfalls and support large models safely. |
| src/lib/utils.ts | Removes unused/incorrect utils.isArray helper. |
| tests/serialization/workflow-serialization.spec.ts | Migrates existing tests to options payload usage. |
| tests/serialization/workflow-serialization-options.spec.ts | New tests covering options payload, legacy compatibility, validate opt-out, YAML output knobs, WIP round-trip. |
| tests/classes/array-hydrator.spec.ts | New regression tests for ArrayHydrator edge cases and generated subclasses. |
| README.md | Documents deserialization validation opt-out and the new serialization options payload + YAML options. |
| examples/node/using-plain-object.ts | Updates JSON serialization example to use { format: 'json' }. |
| examples/node/using-json.ts | Updates JSON serialization example to use { format: 'json' }. |
| examples/node/using-fluent-api.ts | Updates JSON serialization example to use { format: 'json' }. |
| examples/node/using-class.ts | Updates JSON serialization example to use { format: 'json' }. |
| examples/browser/using-plain-object.html | Updates JSON serialization example to use { format: 'json' }. |
| examples/browser/using-json.html | Updates JSON serialization example to use { format: 'json' }. |
| examples/browser/using-fluent-api.html | Updates JSON serialization example to use { format: 'json' }. |
| examples/browser/using-class.html | Updates JSON serialization example to use { format: 'json' }. |
| examples/browser/umd/using-plain-object.html | Updates JSON serialization example to use { format: 'json' }. |
| examples/browser/umd/using-json.html | Updates JSON serialization example to use { format: 'json' }. |
| examples/browser/umd/using-fluent-api.html | Updates JSON serialization example to use { format: 'json' }. |
| examples/browser/umd/using-class.html | Updates JSON serialization example to use { format: 'json' }. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lornakelly
approved these changes
Aug 20, 2026
Contributor
|
@JBBianchi Version should be bumped as well? |
ricardozanini
approved these changes
Aug 20, 2026
Collaborator
Author
It already has been in the previous PR ;) |
toSerializationOptions keyed the deprecated positional form off the presence of a format, but that argument was optional, so `serialize(undefined, false)` was read as an empty payload and normalized anyway. It now keys off the absence of an options payload. toYamlDumpOptions threw on a null yaml block, and the non-mapping deserialization error reported null as 'object'. Signed-off-by: Jean-Baptiste Bianchi <jb.bianchi@neuroglia.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Many thanks for submitting your Pull Request ❤️!
What this PR does / why we need it:
Closes #309.
serialize()was heading toward a growing list of positional booleans, while both serialization and deserialization always validated. That does not work for the editor, where a document is expected to be invalid while it is being edited. A half-written workflow still needs to be saved, reopened, and passed around.Today the editor cannot do that through the SDK, so it falls back to
yaml.load()+new Classes.Workflow(...), which is exactly the code the SDK should be handling. Addingvalidateas another positional flag would also have given us calls likeserialize('yaml', true, false), with three booleans and no useful clue at the call site what any of them mean.SerializationOptionscontainsformat,normalize,validate, and ayamlblock for YAML-specific output options.DeserializationOptionssharesvalidatethrough a smallValidationOptionsbase.formatandnormalizeare intentionally not part ofDeserializationOptions.yaml.load()already accepts both YAML and JSON, so there is no format choice to make on read. Normalizing during deserialization would also rewrite the caller's document as part of loading it, which should stay an explicit operation.The
yamlblock is our own public type, not js-yaml'sDumpOptions. That matters because js-yaml changed that API between v4 and v5:quotingTypebecamequoteStyle,noArrayIndentbecameseqNoIndent,lineWidthmoved ontoPresenterOptions, andflowLevelchanged default from2to-1. If we re-exportedDumpOptions, the js-yaml 4 to 5 upgrade would also have changed our public API. #308 is a fairly good demonstration of why I do not want dependency internals leaking through this boundary.I also checked the built
.d.ts:DumpOptionsappears nowhere except a doc comment.It also fixes three live
ArrayHydratorbugs, because turning validation off makes those paths reachable.The constructor used
!isNaN(model)to distinguish "array length" from "array contents".isNaN()coerces first, so this has some surprising results:Number([])is0, andNumber(['5'])is5.new TaskList([])[[]], one element containing an empty array[]new TaskList(null)[null][]new TaskList({})TypeErrorfrom the spread'The provided model should be an array'new TaskList('ab')['a', 'b']Generated subclasses happened to hide the populated cases because they splice the base-class contents away and push hydrated elements back in. That only happens for a non-empty model, though, and schema validation hid the remaining cases. With validation disabled, an empty task list from the editor could therefore round-trip from
[]to[[]].Special notes for reviewers:
The options overload must stay first, and positional
formatmust stay optional. A bareserialize()should resolve to the new options overload, otherwise editors show the call as deprecated. I first made the legacyformatparameter required to force that resolution, but that breaks perfectly valid forwarding code such asserialize(wf, fmt)whenfmtis typed as'yaml' | 'json' | undefined. That producesTS2769. Declaring the options overload first is enough, soformat?stays optional and there is now a test for that exact case. The existing compatibility tests only used literal arguments, so they would not have caught it.The non-mapping check in
deserialize()only runs when validation is skipped. Without it,deserialize('- a\n- b', { validate: false })hydrates into a blankWorkflowand loses the input because the hydrator ignores non-mapping values. The normal validation path is left alone, so callers that do not opt out still get the sameWorkflowValidationErroras before.yaml.indentis clamped to at least 1. js-yaml v4 did this internally; v5 no longer does. Withindent: 0, nested mappings are emitted at column 0 and parse back into a different document, for example{"document": null, "dsl": "1.0.3", ...}. That is data corruption, not just ugly YAML. Negative values can also throwRangeErrorinside the dumper. Both cases are covered.nullis treated the same as "not set" for options. The UMD bundle is used from plain JavaScript, sonullis a real input even if TypeScript would not normally produce it.serialize()previously used destructuring defaults, which only apply toundefined, whiledeserialize()used??. That meant{ validate: null }disabled validation on write but enabled it on read. Both paths now use??.The regression tests are split by what they actually prove. Reverting the
ArrayHydratorconstructor fails 11 tests. ForcingshouldValidateback totruein generatedserialize()fails 22. TheArrayHydratorcases test the base class directly because that is where the bad single-element state is visible. A generated subclass repairs that state immediately by splicing and re-pushing its contents, so testing only subclasses would hide the constructor bug.src/lib/generated/classes/workflow.tswas regenerated, not hand-edited. The actual source change is intools/4_generate-classes.ts. The generated file belongs in the same commit socodegen:checksees a consistent template and output at every revision.utils.isArrayis removed. Nothing insrc/,tools/,tests/, orscripts/references it, and it is not exported from the package entry point. It also uses the same coercion we are removing here:isArray([])isfalseandisArray({})istrue, despite being declared as a type predicate. There is no reason to leave that around for somebody to call later.Additional information (if needed):
Builds on #311.
serialize()usesasPlainObject(), added as part of the #308 fix.No version bump.
1.0.3-alpha8has not been published. I checked npm and the latest release is stillalpha7. #308 already bumped the package toalpha8, so this can ship in the same unreleased version. Bumping again would just leave an emptyalpha8behind.Verification.
npm testpasses 207 tests across 21 files, up from 137/19.lint,typecheck,build, andvalidate:packageare all clean. Codegen parity was checked offline:tools:4+formatleaves onlyworkflow.tschanged, and repeated runs are byte-identical. End to end,npx tsx examples/node/using-fluent-api.tsstill prints block YAML.Two deliberate non-goals, both decided rather than overlooked.
No
noRefsoption. Add explicitSerializationOptions/DeserializationOptionspayloads, with an opt-out for validation #309 suggested{ lineWidth: -1, noRefs: true }, butnoRefscannot currently change SDK output.asPlainObject()goes through JSON, which removes shared object identity beforeyaml.dump()sees the document, so the dumper has nothing it could turn into an&ref_0anchor. I verified that against js-yaml 5.3.0. There is also a test asserting that SDK serialization does not emit anchors, so ifasPlainObject()changes later we will have a failing test telling us to revisit this.No JSON output options. The
yamlblock is only used whenformat: 'yaml'. If JSON-specific output options become useful later, adding a separate block is straightforward and does not affect this API.Worth follow-ups, out of scope here.
output: nullhydrates to{}.typeof null === 'object', so checks likeif (typeof model.output === 'object')treatnullas something to hydrate. That exists inworkflow.tsand roughly 40 sibling generated classes. It is the object-side equivalent of theArrayHydratorholes fixed here, and becomes easier to reach once validation can be disabled. I left it out because the real fix is intools/reflection.tsand regenerates roughly 90 files, which would bury the changes in this PR.Generated array classes currently copy their model twice. The base class fills itself from the model, then the generated subclass immediately removes those values and pushes hydrated versions back in. That predates this PR, but codegen could avoid the extra work.
BuildOptionsduplicates{ validate, normalize }and is not publicly exported. Add explicitSerializationOptions/DeserializationOptionspayloads, with an opt-out for validation #309 deliberately did not reuse it as the publicserialize()options type, which I still think is the right call. It could still shareValidationOptionsinternally so the relationship exists in the type system instead of only being documented.