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

Fixed type compatibility with typegen-less functions #3051

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Feb 15, 2022

fixes #3033
fixes #3021
fixes #3052
and addresses #2984

@changeset-bot
Copy link

changeset-bot bot commented Feb 15, 2022

🦋 Changeset detected

Latest commit: 76e364a

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

This PR includes changesets to release 1 package
Name Type
xstate 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

@ghost
Copy link

ghost commented Feb 15, 2022

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@codesandbox-ci
Copy link

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.

Latest deployment of this branch, based on commit 76e364a:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration


Fixed type compatibility with functions accepting machines that were created before typegen was a thing in XState. This should make it possible to use the latest version of XState with `@xstate/vue`, `@xstate/react@^1` and some community packages.

Note that this change doesn't make those functions to accept machines that have typegen information on them. For that the signatures of those functions would have to be adjusted.
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't possible because we are using __TResolvedTypesMeta phantom type here and this makes a machine with typegen information incompatible with a machine that has typegen information disabled for it.

Maybe, maybe it would be possible to fix this by defaulting to any and pushing resolving logic for the TypegenDisabled case further down the road. This would require using IsAny and stuff like that and, in general, I've experienced anys wreaking havoc on the type machine. Maybe it can be wrangled but I'm somewhat scared of doing such a substantial change now cause while this type machinery works quite alright... it's somewhat fragile.

I think that, for the time being, this is a reasonable tradeoff for what the typegen enables us to do. It feels better to push it harder than to experiment with changes that might break it. I imagine that with some of the changes planned for v5 this might become much less of a problem - but also I can't be certain about it until we actually try to minimize the amount of generics etc.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to create an AnyTypegenMachine type to help with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned in the private conversation - this wouldn't work correctly because the whole type machinery is built on top of a type that has some specific structure. ResolvedTypegenDisabled is acting, sort of, like UnknownTypegenMeta. What I mean by this is that the typegen meta includes string literals etc when the ResolvedTypegenDisabled has just string, Record<string, unknown> etc. In a way probably this holds true:

const truthy: ResolvedTypegenMeta extends ResolvedTypegenDisabled ? true : false = true

This allows the whole machine after the initial resolution to work exactly the same - for the most part, I don't even need to check if the resolved type contains metadata created by the typegen or not, I don't branch the implementation based on this.

This is just part of the problem though. The first problem that would have to be addressed before even thinking about any kind of reactors here is related to machine.withConfig. This method utilizes the typegen information and might mark some implementations as required. If the consuming function doesn't specify the typegen slot anyhow we need to default it to something. Whatever we default it to though has to end up marking all implementations provided to withConfig as optional - because this is like the most "loose" shape.

So the consuming function that doesn't specify the typegen slot anyhow will always end up expecting a machine that has .withConfig(options: EverythingOptional) while the typegen-based provides something like .withConfig(options: SomeMightBeRequired). Those types are just fundamentally incompatible because allowing this would be unsound, especially when we consider strictFunctionTypes (and I believe that it's our target to support this option, in a way - I don't intend to work on any special support for the opposite).

So it feels like the only viable solution here is to enforce the choice on those consuming functions:

  1. they can either provide any for the typegen slot to say "I don't care about this as I don't do anything special with it"
  2. or they can infer it or provide a generic for it to utilize the information or forward it to the result of their function or something

@Andarist Andarist merged commit 04091f2 into main Feb 16, 2022
@Andarist Andarist deleted the andarist/fix-types-compat branch February 16, 2022 11:03
@github-actions github-actions bot mentioned this pull request Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants