-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Machine JSON Schema definition #960
Conversation
🦋 Changeset is good to goLatest commit: 55aa589 We got this. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
@Andarist Is this good to go? We'll make refinements later but this doesn't change any behavior |
Sorry, I've missed this being promoted to a pull request from a draft, so I've missed that I can get to reviewing it. |
} | ||
}, | ||
"required": ["type", "id", "src"], | ||
"additionalProperties": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to test this JSON schema against my implementation.
I got a validation issue because my invoke
contains onDone
and onError
as additional properties.
Error:
{ keyword: 'additionalProperties',
dataPath: '.states[\'f941522f-a625-4953-9472-7d3c4004d1d4\'].invoke[0]',
schemaPath: '#/definitions/invokeObject/additionalProperties',
params: { additionalProperty: 'onError' },
message: 'should NOT have additional properties' },
Object:
states: {
[myId]: {
id: 'doSomething',
key: myId,
type: 'atomic',
invoke: [
{
src: 'doSomething',
onDone: `${myId}_EXECUTED`,
onError: `${myId}_FAILED`,
},
],
}
https://xstate.js.org/docs/guides/communication.html#the-invoke-property
Should you add onDone
and onError
in the schema as well or I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure - but maybe you test the wrong thing? onError & onDone are part of the config but they are converted to transitions internally and thus probably available on the definition (and not the config)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it makes sense. Indeed, I'm creating a config so I can't test against this JSON schema. I will wait for this feature to get merge then
'xstate': minor | ||
--- | ||
|
||
The machine can now be safely JSON-serialized, using `JSON.stringify(machine)`. The shape of this serialization is defined in `machine.schema.json` and reflected in `machine.definition`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have meant that machine.defiinition
can be safely serialized - as that is what you are doing in tests:
https://github.com/davidkpiano/xstate/blob/55aa589648a9afbd153e8b8e74cbf2e0ebf573fb/packages/core/test/json.test.ts#L85
I would be in favor of keeping machine.definition
serializable, rather than the machine itself. IMHO makes it more explicit what actually gets serialized, especially that toJSON put on a Machine would just proxy to serializing .definition
This PR validates the machine against a JSON schema.