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

Capture any string in the meta field of a machine or state #225

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abrgr
Copy link

@abrgr abrgr commented Sep 29, 2022

Currently, only description is captured from meta. It would be super helpful to capture any string meta field.

@changeset-bot
Copy link

changeset-bot bot commented Sep 29, 2022

🦋 Changeset detected

Latest commit: 47ac2a6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@xstate/machine-extractor Minor
@xstate/tools-shared Patch
@xstate/cli Patch
stately-vscode Patch

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

@Andarist
Copy link
Member

What does this solve? Where do you expect to see those extra keys?

@abrgr abrgr force-pushed the allow-any-string-value-in-meta branch from 609baff to 47ac2a6 Compare September 29, 2022 22:05
@abrgr
Copy link
Author

abrgr commented Sep 29, 2022

At the moment, I would like to use them here to generate gherkin tests from an xstate statechart. Meta keys are generally useful for any type of statechart reflection like this. If they are captured by the machine-extractor, it becomes much easier to build these types of tools that rely on state metadata.

@davidkpiano
Copy link
Member

This is a favorable change, since meta.description should be deprecated anyway in favor of description.

@Andarist
Copy link
Member

I'm just slightly worried about the ripple effect of this change at the moment. I'm unsure how the previous thing was exactly utilized by our system. The current API of this package is really coupled at times to the underlying AST - and in fact, this change is a breaking change for this package.

I don't mind this change but I would have to do a deeper analysis to see if this would be a breaking change for us.

@abrgr
Copy link
Author

abrgr commented Sep 29, 2022

Totally makes sense. Let me know if I can be helpful in analyzing that.

If it helps, the previous behavior was to capture meta.description but to ignore any other field on meta. So if a file had a machine that defined

meta: {
  abc: "abc"
}

parseMachinesFromFile would return a machine/state without a meta object.

If a file had a machine that defined:

meta: {
  description: "my description",
  abc: "abc"
}

parseMachinesFromFile would return a machine/state with meta of only { description: "my description" }.

With this change, you may get a meta without a description (which may not have been expected previously). That is the main way that this could break existing code.

However, based on the type returned from objectTypeWithKnownKeys (what was previously used to populate meta), I think that meta.description should always have been optional, though in practice, it was never null/undefined as previously implemented.

@Andarist
Copy link
Member

The breaking change I had in mind is about the shape of the extracted node - unfortunately, the AST shape leaks there. Now you have .properties property there, whereas previously it was .description.

@abrgr
Copy link
Author

abrgr commented Oct 10, 2022

@Andarist - 2 thoughts:

  1. I can make StateMeta in packages/machine-extractor/src/meta.ts a proxy that throws on any access to .description and run any tests or anything you had in mind to see if that's accessed.
  2. I can modify StateMeta in packages/machine-extractor/src/meta.ts to return an AST with both .properties and .description if .description exists.

Would either of these be helpful?

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.

3 participants