Skip to content

feat: add (de)serialization options with a validation opt-out - #312

Merged
JBBianchi merged 2 commits into
open-workflow-specification:mainfrom
neuroglia-io:feat-309-add-serialization-options
Aug 20, 2026
Merged

feat: add (de)serialization options with a validation opt-out#312
JBBianchi merged 2 commits into
open-workflow-specification:mainfrom
neuroglia-io:feat-309-add-serialization-options

Conversation

@JBBianchi

Copy link
Copy Markdown
Collaborator

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. Adding validate as another positional flag would also have given us calls like serialize('yaml', true, false), with three booleans and no useful clue at the call site what any of them mean.

serialize(options?: SerializationOptions): string

deserialize(text: string, options?: DeserializationOptions): WorkflowIntersection

SerializationOptions contains format, normalize, validate, and a yaml block for YAML-specific output options. DeserializationOptions shares validate through a small ValidationOptions base.

format and normalize are intentionally not part of DeserializationOptions. 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 yaml block is our own public type, not js-yaml's DumpOptions. That matters because js-yaml changed that API between v4 and v5: quotingType became quoteStyle, noArrayIndent became seqNoIndent, lineWidth moved onto PresenterOptions, and flowLevel changed default from 2 to -1. If we re-exported DumpOptions, 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: DumpOptions appears nowhere except a doc comment.

It also fixes three live ArrayHydrator bugs, 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([]) is 0, and Number(['5']) is 5.

before after
new TaskList([]) [[]], one element containing an empty array []
new TaskList(null) [null] []
new TaskList({}) raw TypeError from the spread 'The provided model should be an array'
new TaskList('ab') ['a', 'b'] same error

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 format must stay optional. A bare serialize() should resolve to the new options overload, otherwise editors show the call as deprecated. I first made the legacy format parameter required to force that resolution, but that breaks perfectly valid forwarding code such as serialize(wf, fmt) when fmt is typed as 'yaml' | 'json' | undefined. That produces TS2769. Declaring the options overload first is enough, so format? 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 blank Workflow and 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 same WorkflowValidationError as before.

  • yaml.indent is clamped to at least 1. js-yaml v4 did this internally; v5 no longer does. With indent: 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 throw RangeError inside the dumper. Both cases are covered.

  • null is treated the same as "not set" for options. The UMD bundle is used from plain JavaScript, so null is a real input even if TypeScript would not normally produce it. serialize() previously used destructuring defaults, which only apply to undefined, while deserialize() 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 ArrayHydrator constructor fails 11 tests. Forcing shouldValidate back to true in generated serialize() fails 22. The ArrayHydrator cases 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.ts was regenerated, not hand-edited. The actual source change is in tools/4_generate-classes.ts. The generated file belongs in the same commit so codegen:check sees a consistent template and output at every revision.

  • utils.isArray is removed. Nothing in src/, tools/, tests/, or scripts/ references it, and it is not exported from the package entry point. It also uses the same coercion we are removing here: isArray([]) is false and isArray({}) is true, 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() uses asPlainObject(), added as part of the #308 fix.

No version bump. 1.0.3-alpha8 has not been published. I checked npm and the latest release is still alpha7. #308 already bumped the package to alpha8, so this can ship in the same unreleased version. Bumping again would just leave an empty alpha8 behind.

Verification. npm test passes 207 tests across 21 files, up from 137/19. lint, typecheck, build, and validate:package are all clean. Codegen parity was checked offline: tools:4 + format leaves only workflow.ts changed, and repeated runs are byte-identical. End to end, npx tsx examples/node/using-fluent-api.ts still prints block YAML.

Two deliberate non-goals, both decided rather than overlooked.

  • No noRefs option. Add explicit SerializationOptions / DeserializationOptions payloads, with an opt-out for validation #309 suggested { lineWidth: -1, noRefs: true }, but noRefs cannot currently change SDK output. asPlainObject() goes through JSON, which removes shared object identity before yaml.dump() sees the document, so the dumper has nothing it could turn into an &ref_0 anchor. I verified that against js-yaml 5.3.0. There is also a test asserting that SDK serialization does not emit anchors, so if asPlainObject() changes later we will have a failing test telling us to revisit this.

  • No JSON output options. The yaml block is only used when format: '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: null hydrates to {}. typeof null === 'object', so checks like if (typeof model.output === 'object') treat null as something to hydrate. That exists in workflow.ts and roughly 40 sibling generated classes. It is the object-side equivalent of the ArrayHydrator holes fixed here, and becomes easier to reach once validation can be disabled. I left it out because the real fix is in tools/reflection.ts and 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.

  • BuildOptions duplicates { validate, normalize } and is not publicly exported. Add explicit SerializationOptions / DeserializationOptions payloads, with an opt-out for validation #309 deliberately did not reuse it as the public serialize() options type, which I still think is the right call. It could still share ValidationOptions internally so the relationship exists in the type system instead of only being documented.

Signed-off-by: Jean-Baptiste Bianchi <jb.bianchi@neuroglia.io>
@JBBianchi
JBBianchi requested a lite review from Copilot August 20, 2026 15:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in serialize() and deserialize(), while keeping the legacy positional signature (deprecated).
  • Fix ArrayHydrator constructor 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.

Comment thread src/lib/serialization.ts Outdated
Comment thread src/lib/serialization.ts Outdated
Comment thread tools/4_generate-classes.ts
Comment thread src/lib/hydrator.ts

@lornakelly lornakelly left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - aside from copilot comments

@lornakelly

Copy link
Copy Markdown
Contributor

@JBBianchi Version should be bumped as well?

@JBBianchi

Copy link
Copy Markdown
Collaborator Author

@JBBianchi Version should be bumped as well?

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>
@JBBianchi
JBBianchi merged commit 8d19e85 into open-workflow-specification:main Aug 20, 2026
2 checks passed
@JBBianchi
JBBianchi deleted the feat-309-add-serialization-options branch August 20, 2026 17:51
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

Successfully merging this pull request may close these issues.

Add explicit SerializationOptions / DeserializationOptions payloads, with an opt-out for validation

4 participants