mattpocock / xstate-codegen

A codegen tool for 100% TS type-safety in XState
MIT License
245 stars 12 forks source link

Allowed for aliasing services to serviceName.onDone and serviceName.onError #41

Closed mattpocock closed 3 years ago

mattpocock commented 3 years ago

Potential fix for #33

changeset-bot[bot] commented 3 years ago

🦋 Changeset is good to go

Latest commit: 85ad5e5ae7eb6134a8b49b85b5c60bc7124e54f0

We got this.

This PR includes changesets to release 1 package | Name | Type | | -------------- | ----- | | xstate-codegen | Minor |

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 commented 3 years ago

@Andarist we've had some internal chats about this PR. Our ideas have centered around:

Can we infer this kind of information without the user passing it?

Possibly, but only if the service is declared either inline, or in the second parameter of Machine/createMachine.

This API doesn't seem right

I partially agree - we're really just adding an alias from done.invoke.serviceName to serviceName.onDone. This is potentially confusing. I think I'm on the side of "this is the best idea we have, let's go with it". So I wanted to open this up to see if anyone else has any decent ideas.

Let me know if I've missed anything above. I like the above PR, but I feel like there might be a better solution.

mwarger commented 3 years ago

For what it's worth, I think that it's good enough. I think this would be a good thing to ship until we can come up with something better.

Andarist commented 3 years ago

I will just copy-paste my arguments against this from the Slack, so it's all kept in the single place here:

I think that especially the 3rd one is strong (1st one is subjective, but quite strong for me 😅 ).

I partially agree - we're really just adding an alias from done.invoke.serviceName to serviceName.onDone. This is potentially confusing.

This is confusing also because one won't ever receive such a declared event but yet can receive the original one. So this might come as a surprise for somebody when debugging etc.

Possibly, but only if the service is declared either inline, or in the second parameter of Machine/createMachine.

Actually, that's not quite true... @mwarger had an idea about how this could be achieved in its current form. The proposed solution imposes some constraints on the user, but given the understandable pushback regarding only allowing all-in-one-place machine definitions maybe it's the way to go? To the point though. @mwarger has suggested to crawl the whole project in the search of references to the created machine, grab types from there, and fill the gaps at the definition site. We have much more freedom with the approach of having a tool to do the codegen/typegen stuff than when when only can rely on TS builtin inference. One caveat of the proposed solution is to restrict consuming sites to 1 because if one provides more than one set of some stuff at different consuming sites then I'm really not sure how to generate stuff reliably. The complexity of this doesn't sound justified at all - even if we could figure something out. Alternatively we could allow for multiple consuming sites but we'd have to validate if they provide functions with the same signature etc.

mattpocock commented 3 years ago

This is confusing also because one won't ever receive such a declared event but yet can receive the original one. So this might come as a surprise for somebody when debugging etc.

Gotcha - yes, this in particular is a killer. If you're looking inside the XState inspector, you won't see service.onDone but you will see done.invoke.service. That sort of indirection can put you off a tool.

Scraping the call sites for services makes sense. I'm happy to close this issue (since it already has a workaround).