Skip to content
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

Merged
merged 12 commits into from
Feb 24, 2020
Merged

Machine JSON Schema definition #960

merged 12 commits into from
Feb 24, 2020

Conversation

davidkpiano
Copy link
Member

This PR validates the machine against a JSON schema.

@changeset-bot
Copy link

changeset-bot bot commented Jan 24, 2020

🦋 Changeset is good to go

Latest 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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 24, 2020

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.

@davidkpiano davidkpiano marked this pull request as ready for review February 1, 2020 18:02
@davidkpiano
Copy link
Member Author

@Andarist Is this good to go? We'll make refinements later but this doesn't change any behavior

packages/core/test/json.test.ts Show resolved Hide resolved
packages/core/test/json.test.ts Show resolved Hide resolved
packages/core/test/json.test.ts Show resolved Hide resolved
packages/core/src/types.ts Show resolved Hide resolved
packages/core/src/StateNode.ts Outdated Show resolved Hide resolved
packages/core/src/json.ts Show resolved Hide resolved
@Andarist
Copy link
Member

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
Copy link

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?

Copy link
Member

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)

Copy link

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`.
Copy link
Member

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

@davidkpiano davidkpiano merged commit 189c928 into master Feb 24, 2020
@github-actions github-actions bot mentioned this pull request Feb 24, 2020
@Andarist Andarist deleted the davidkpiano/json-schema branch February 24, 2020 21:58
@github-actions github-actions bot mentioned this pull request Mar 4, 2020
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.

None yet

4 participants