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

fix: bug in typings after recent PR #95

Merged
merged 3 commits into from
Apr 4, 2023
Merged

Conversation

ffMathy
Copy link
Contributor

@ffMathy ffMathy commented Apr 4, 2023

  • I have added automatic tests where applicable
  • The PR title is suitable as a release note
  • The PR contains a description of what has been changed
  • The description contains manual test instructions

Changes

I removed the Unknown event type. It ruins the type discrimination it seems. Not sure how this was not caught by me earlier. Apologies. Didn't discover this until I had to consume the event myself.

Test

Test that the types are still OK.

@ffMathy ffMathy requested a review from a team as a code owner April 4, 2023 06:30
@gismya
Copy link
Contributor

gismya commented Apr 4, 2023

The problem is that there are other valid event types than the ones currently typed, and they are unknown. Neither solution is great, and I'm now sure which one is worst. If only TypeScript had negated types.

We could examine if the new const in typescript 5 would allow anything useful here. But if we're stuck with the imperfect way it currently works I'd love arguments on this solution rather than type-casting.

And if we go with this we should probably add some comments about how any custom events would have to have their own interface created for them when used.

Does @jimmycallin have any thoughts? I know you've struggled with the lack of negated types before.

@ffMathy
Copy link
Contributor Author

ffMathy commented Apr 4, 2023

I think the way forward is to define all of the event types to be honest. And I don't think the current state of things in this PR is very bad. I think the alternative is worse.

Copy link
Contributor

@gismya gismya left a comment

Choose a reason for hiding this comment

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

I'm inclined to agree that unknown types breaking the type discrimination might be worse. I have some ideas on how to make it more easily extendable and versatile, but that would have to wait until the event typing overhaul I mentioned in the previous PR.

Added a comment suggestion for informing about the incompleteness of the type definitions.

source/event_hub.ts Show resolved Hide resolved
Co-authored-by: Lars Johansson <[email protected]>
@ffMathy
Copy link
Contributor Author

ffMathy commented Apr 4, 2023

That's a great idea.

How many event types are there in Ftrack, and does it take too long to make them all? Can they somehow be auto-generated?

@gismya
Copy link
Contributor

gismya commented Apr 4, 2023

The event hub forwards any event sent to it, with arbitrary topics and arbitrary data. They have been added when they were needed from whatever part of the stack needed them. That's why it takes some research to find them all, document what data they contain, and whether they are public events or just events we use internally. So I'm not sure how many there are that are missing, but from the previous investigation I started there's about a handful.

@gismya gismya merged commit b2e7961 into ftrackhq:main Apr 4, 2023
@ffMathy
Copy link
Contributor Author

ffMathy commented Apr 4, 2023

I am an idiot. I think I forgot to bump the version in package.json. So it was not released.

@gismya
Copy link
Contributor

gismya commented Apr 4, 2023

We don't manually bump, that is done through the release scripts. I'll check what went wrong.

edit: I accidentally named the version v.1.4.6 instead of v1.4.6. Now it should be working.

@ffMathy
Copy link
Contributor Author

ffMathy commented Apr 4, 2023

Thanks a lot!

gismya added a commit to gismya/ftrack-javascript that referenced this pull request Apr 11, 2023
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.

2 participants