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

Eliminate `NonEmptyObject` #2152

Closed thegedge closed 4 months ago

thegedge commented 4 months ago

What does this PR do and why?

Resolves https://github.com/mobxjs/mobx-state-tree/issues/1524

NonEmptyObject was introduced in https://github.com/mobxjs/mobx-state-tree/pull/1269 to prevent passing in any object to a model type that had no props.

Although this works great, some people have found it harder to work with because now keyof typeof MyModelType now has this extra key they have to deal with. One could intersect that type with string, but it's unfortunate that users have to do this.

Instead, we use Record<string, never> which can only be represented by an empty object literal. Snapshot preprocessors also had to be tweaked because they can emit properties that aren't known to them in the case of composition with other types.

Steps to validate locally

New test added to prove this works, and ran the existing test suite to ensure no breakages.

thegedge commented 4 months ago

CI seems to be failing, but it passes locally for me (as you said it did for you, too). I tried re-running, but no luck.

Yeah, looks like it's typechecking locally, but not when I open the specific file. I think this is some misconfiguration of tsconfig.json (I'll follow up with a PR to deal with that).

thegedge commented 4 months ago

Looks like that last attempt failed too. I'll keep poking. It's possible that we ended up at this $nonEmptyObject solution because it was the only one to satisfy all of the requirements (baked into the existing tests).

thegedge commented 4 months ago

Fixed. Added in some more checks and comments to hopefully help explain things.

coolsoftwaretyler commented 4 months ago

LGTM! Thanks again for all the hard work.