search

mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.9k stars 640 forks source link

Improved typing for union types #2151

Closed thegedge closed 4 months ago

thegedge commented 4 months ago

What does this PR do and why?

After 9 or 10 types in a union, we devolve into an IAnyType. This isn't particularly useful, and potentially even dangerous (in a typechecking sense, given the behaviour of any).

This PR makes union types become proper TS sum types for any number of member types given to the union. It also results in types that are much simpler to manage, since we don't have to "unroll" for each possible number of types. We've also brought similar improvements to types.enumeration.

Steps to validate locally

I added some new tests for union and enumeration types, and verified that the tests fail before this change and pass after.

thegedge commented 4 months ago

Although I can't quite explain it, this appears to also resolve https://github.com/mobxjs/mobx-state-tree/issues/1525

On main: CleanShot 2024-02-24 at 18 49 20

On this branch: CleanShot 2024-02-24 at 18 50 15

[EDIT] Same for https://github.com/mobxjs/mobx-state-tree/issues/1664

Haven't dug deeper, but I'm wondering if it'll also knock off https://github.com/mobxjs/mobx-state-tree/issues/1833

coolsoftwaretyler commented 4 months ago

Although I can't quite explain it, this appears to also resolve #1525

On main: CleanShot 2024-02-24 at 18 49 20

On this branch: CleanShot 2024-02-24 at 18 50 15

[EDIT] Same for #1664

Haven't dug deeper, but I'm wondering if it'll also knock off #1833

Hey @thegedge - I can't verify this fix. I'm seeing the issue still in a test in bcac3c0. How did you get the passing screenshot?

coolsoftwaretyler commented 4 months ago

c2b47ee confirms the fix for #1664.

coolsoftwaretyler commented 4 months ago

47c0702 replicates https://github.com/mobxjs/mobx-state-tree/issues/1833 - so I don't think this fixes that quite yet either, unless I've got something borked locally as in this comment

coolsoftwaretyler commented 4 months ago

Merge conflict came from https://github.com/mobxjs/mobx-state-tree/pull/2152 since we're editing __tests__/core/type-system.test.ts in both. Let's resolve some of the other items before we resolve the conflicts. Should be able to pull from both current and incoming on that one, if I'm not mistaken.

thegedge commented 4 months ago

Oh yeah, great call on regression tests for the issues we fixed!

I'll double check on #1525 this evening

coolsoftwaretyler commented 4 months ago

@thegedge - I added a test for 1525, and it's failing in CI

We definitely don't need to block on this, but since it looks like you've seen the fix for 1525 at some point, would be cool to try and verify it.

Let me know whatcha think!

coolsoftwaretyler commented 4 months ago

I will remove that failing test and plan to merge this in sometime over the weekend unless I hear otherwise.

thegedge commented 4 months ago

Hey, @coolsoftwaretyler. Thanks for 👀. We have a big launch week coming up so I've been super busy and neglected to check in. If I can't get to it this evening, let's ignore and follow up with #1525 later.

coolsoftwaretyler commented 4 months ago

Perfect, thank you!

thegedge commented 4 months ago

Amended your test commit, was just missing an important check from the issue to ensure narrowing happens. In this PR, it passes!

🙇 for adding all these tests. I'll keep that in mind for future work.

coolsoftwaretyler commented 4 months ago

Phenomenal!

I'll merge and then put out a pre-release build to npm, along with an RFC about how strict we want to be around semantic versioning here.