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

Widened the *From utility types to allow extracting from factory functions #2551

Conversation

mattpocock
Copy link
Contributor

@mattpocock mattpocock commented Aug 24, 2021

Copied from changeset:

Widened the *From utility types to allow extracting from factory functions.

This allows for:

const makeMachine = () => createMachine({});

type Interpreter = InterpreterFrom<typeof makeMachine>;
type Actor = ActorRefFrom<typeof makeMachine>;
type Context = ContextFrom<typeof makeMachine>;
type Event = EventsFrom<typeof makeMachine>;

This also works for models, behaviours, and other actor types.

@changeset-bot
Copy link

changeset-bot bot commented Aug 24, 2021

🦋 Changeset detected

Latest commit: 6d284fe

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

@mattpocock mattpocock changed the title Widened the From utility types to allow extracting from factory functions Widened the *From utility types to allow extracting from factory functions Aug 24, 2021
@mattpocock mattpocock removed the request for review from farskid August 24, 2021 09:55
Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

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

Really useful change. Wondering if it would be too much abstraction to have a type like this?

type FactoryOrValue<T> = T extends ((...args: any[]) => infer U) ? U : T;

Then we can do:

export type StateFrom<T> = FactoryOrValue<T> extends StateMachine<any,any,any>
  ? FactoryOrValue<T>['transition']
  : ...

Just thinking out loud 🤔

@davidkpiano
Copy link
Member

We can get this merged in after tests are passing (I can help investigate)

@@ -828,7 +828,11 @@ export interface StateMachine<
): StateMachine<TContext, TStateSchema, TEvent, TTypestate>;
}

export type StateFrom<T> = T extends StateMachine<any, any, any>
export type StateFrom<
T extends
Copy link
Member

Choose a reason for hiding this comment

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

Is the idea to add to this extends whenever we add a new thing that StateFrom can extract a state from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that will work.

Copy link
Member

Choose a reason for hiding this comment

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

I've removed this on my branch - https://github.com/mattpocock/xstate/pull/1/files . I don't have super strong opinion about this and definitely having those constraints in place could improve where type errors are reported and so on. We didn't have those in other *From utilities - so I've just gone for the unification for the time being. We can always unify this other way around - although keeping the list of those things is a little bit tedious 🤷‍♂️

@mattpocock
Copy link
Contributor Author

Really useful change. Wondering if it would be too much abstraction to have a type like this?

type FactoryOrValue<T> = T extends ((...args: any[]) => infer U) ? U : T;

Then we can do:

export type StateFrom<T> = FactoryOrValue<T> extends StateMachine<any,any,any>
  ? FactoryOrValue<T>['transition']
  : ...

Just thinking out loud 🤔

Probably a good idea? I'm really nervous to do stuff like this in a codebase that doesn't have strict: true in it, but it would probably work.

@mattpocock
Copy link
Contributor Author

@davidkpiano Tried the above, doesn't work

@Andarist
Copy link
Member

Re FactoryValue - I have tried a slightly different approach and it seems to work: https://github.com/mattpocock/xstate/pull/1/files . It would be great to add some tests for this though.

@davidkpiano
Copy link
Member

Re FactoryValue - I have tried a slightly different approach and it seems to work: https://github.com/mattpocock/xstate/pull/1/files . It would be great to add some tests for this though.

Nice; well, we can do refactor later. I think this is good to go in, wdyt?

@Andarist
Copy link
Member

Nice; well, we can do refactor later. I think this is good to go in, wdyt?

Well, sure - but since it's already implemented, wouldn't it be easier to review it, merge to this one and then land on the main branch?

@davidkpiano
Copy link
Member

Nice; well, we can do refactor later. I think this is good to go in, wdyt?

Well, sure - but since it's already implemented, wouldn't it be easier to review it, merge to this one and then land on the main branch?

Sure, that sounds good to me

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
@davidkpiano davidkpiano merged commit 264679d into statelyai:main Aug 28, 2021
@github-actions github-actions bot mentioned this pull request Aug 28, 2021
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

3 participants