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

Eliminate NonEmptyObject #2152

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

thegedge
Copy link
Collaborator

What does this PR do and why?

Resolves #1524

NonEmptyObject was introduced in #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.

This was introduced in mobxjs#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.
Copy link
Collaborator

@coolsoftwaretyler coolsoftwaretyler left a comment

Choose a reason for hiding this comment

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

Hey @thegedge - this is awesome!

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.

I'm about to turn in for the night, but I can take a look tomorrow and see if I can figure out what's blocking CI.

Once that passes, I'll probably merge into master, and ship it as part of the pre-release build alongside #2151.

Thanks again, this is awesome!

__tests__/core/type-system.test.ts Show resolved Hide resolved
__tests__/core/type-system.test.ts Outdated Show resolved Hide resolved
src/types/complex-types/model.ts Show resolved Hide resolved
@thegedge
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@coolsoftwaretyler
Copy link
Collaborator

LGTM! Thanks again for all the hard work.

@coolsoftwaretyler coolsoftwaretyler merged commit 46334b6 into mobxjs:master Feb 26, 2024
1 check passed
@thegedge thegedge deleted the remove-nonemptyobject branch February 26, 2024 14:12
coolsoftwaretyler pushed a commit that referenced this pull request Feb 26, 2024
* Eliminate `NonEmptyObject`

This was introduced in #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.

* Don't allow symbols in input snapshots for models with no props
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.

TypeScript: how to get type of model properties without internal fields?
2 participants