Fix for empty models / models with all properties set to optional being able to take any value in TypeScript #1269

xaviergonz · 2019-04-29T18:48:10Z

Apparently, in TS this is valid

const x: {} = 5
const x: {} = [5]

which means that for empty objects / objects where all properties were optional any value was valid

This PR fixes this by turning empty objects into an object like

{
  [some unique symbol]?: undefined
}

which doesn't have any properties in common with array, classes, etc.

Also updates some dev deps and fixes some typos

coveralls · 2019-04-29T18:54:18Z

Coverage remained the same at 92.465% when pulling 6b966be on fix-1268 into 8ef9c35 on master.

mweststrate · 2019-04-30T17:47:48Z

packages/mobx-state-tree/src/types/complex-types/model.ts

 /** @hidden */
 export type ExtractCFromProps<P extends ModelProperties> = { [k in keyof P]: P[k]["CreationType"] }

 /** @hidden */
-export type ModelCreationType<PC> = { [P in DefinablePropsNames<PC>]: PC[P] } & Partial<PC>
+export type KeylessToEmptyObject<O> = keyof O extends never ? EmptyObject : O


keyof O extends never 💯

xaviergonz · 2019-04-30T18:08:26Z

While it works I'm going to mark it as WIP since merging this would make the 50 iterations error from TS3.4 appear again :/

Hopefully this new feature from TS (microsoft/TypeScript#30637) means what I think it means and then it might even resolve by itself.

Anyway it only exhibits in two cases

when the model has no props the creation/snapshot/instance types accept anything
when all model props are optional then creation accepts arrays and numbers, but not wrong objects, while the other types seem ok

xaviergonz · 2019-04-30T18:25:39Z

Ok, i just checked with the next TSC and it doesn't appear to mean what I think :-/ (unless it wasn't still there in the nightly build)

mweststrate · 2019-04-30T18:27:24Z

If it is troublesome with TS 3.4, I would wait a bit before merging this

…

On Tue, Apr 30, 2019 at 8:08 PM Javier Gonzalez ***@***.***> wrote: While it works I'm going to mark it as WIP since merging this would make the 50 iterations error from TS3.4 appear again :/ Hopefully this new feature from TS (microsoft/TypeScript#30637 <microsoft/TypeScript#30637>) means what I think it means and then it might even resolve by itself. Anyway it only exhibits in two cases 1. when the model has no props the creation/snapshot/instance types accept anything 2. when all model props are optional then creation accepts arrays and numbers, but not wrong objects, while the other types seem ok — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1269 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBB2JQ5X7GTOUN5PVHTPTCDKBANCNFSM4HJFZUBQ> .

xaviergonz · 2019-05-01T15:33:27Z

managed to reduce the number of iterations to 46, but still too high I think.

Maybe it would be better to change types.model typings so it cannot take an empty object as props?
The problem would still be there when all props are optional though

mweststrate · 2019-05-02T12:55:55Z

Maybe it would be better to change types.model typings so it cannot take an empty object as props?

Sounds as a good idea anyway

The problem would still be there when all props are optional though

That is actually quite a common case I presume 😢

mweststrate · 2019-07-19T19:20:05Z

I'm not sure if the issue is big enough to warrant the additional complexity in the typings. Ok to close this one?

xaviergonz · 2019-07-19T19:36:15Z

I'd like to try some ideas I had to reduce the number of types nesting before giving up on this one.

mweststrate · 2019-07-19T19:38:39Z

np!

…

On Fri, Jul 19, 2019 at 9:36 PM Javier Gonzalez ***@***.***> wrote: I'd like to try some ideas I had to reduce the number of types nesting before giving up on this one. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1269?email_source=notifications&email_token=AAN4NBGIKOYCTQIUB5TDZVDQAIJTJA5CNFSM4HJFZUB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2MRZLY#issuecomment-513350831>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBAO3TSRKOSDGZ5OF7LQAIJTJANCNFSM4HJFZUBQ> .

danielkcz · 2019-07-19T19:48:05Z

I still haven't got to proper upgrading to 3.5 due to #1341, but I am expecting this is gonna bite me too. I do have a couple of models with all props optional. I wonder what's wrong with the PR? Does it work with that? Doesn't seem to be overly complicated, so I am not sure why to abandon it.

xaviergonz · 2019-07-19T19:54:11Z

The problem has to do with a limit (added in TS 3.4 I think?) on how deep conditional types can go, which is 50 levels deep. Some medium-big projects where reaching that limit and that got fixed, but this PR adds some more levels and makes it go over 50 again, thus making these medium-big projects un-compilable.

I was just thinking of trying to leverage interfaces as "cached" types to reduce nesting, something like

interface A<X> {
  $whatever: X extends Foo ? Bar : Whatever // imagine some super complex conditional type here
}

types Ble<X> = X extends A<X> ? X["$whatever"] : never

rather than directly

types Ble<X> = X extends Foo ? Bar : Whatever

something like that (although it is an oversimplification), but I still have to check if that actually reduces nesting

xaviergonz · 2019-07-20T11:27:48Z

@mweststrate Ok I found a better way to fix it and not increase the depth levels at all, basically by always '&' a non empty object like { [someSymbol]?: any }

Also while at it updated the dev deps, which includes a nice update to the typedocs markdown plugin, which should result in a bit better looking autogenerated docs

k-g-a

LGTM

danielkcz · 2019-10-17T16:54:34Z

@xaviergonz We got some approvals here. I suppose you can resolve conflicts and merge? :)

xaviergonz · 2019-10-17T17:11:10Z

Should be fixed.
While at it also update deps and fixed flow typings for TS3.6, but this means that now flows require TS3.6.

xaviergonz · 2019-10-17T17:12:26Z

@mweststrate since the fix for flow typings require ts3.6, is it ok to merge?

mweststrate · 2019-10-18T19:38:37Z

I think it is OK if it is mentioned in the changelog, and at least a minor release

Since mobxjs#1269 marked `castFlowReturn` as deprecated, it shouldn't be mentioned as needed in the docs.

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.

* 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

xaviergonz added 2 commits April 29, 2019 20:36

fix ts for empty objs

13906a3

fix circleci, update changelog

39072ad

This was referenced Apr 29, 2019

s/onInvalidate/onInvalidated #1266

Closed

sp #1265

Closed

better fix

0ab8489

xaviergonz requested review from mweststrate and k-g-a April 29, 2019 20:07

mweststrate reviewed Apr 30, 2019

View reviewed changes

mweststrate approved these changes Apr 30, 2019

View reviewed changes

xaviergonz changed the title ~~Fix for empty models / models with all properties set to optional being able to take any value in TypeScript~~ Fix for empty models / models with all properties set to optional being able to take any value in TypeScript [WIP] Apr 30, 2019

xaviergonz added 2 commits May 1, 2019 11:19

small optimization

d258b15

merge master

5cb282f

This was referenced Jun 5, 2019

Optional default gets a Typescript complaint #1308

Closed

Issues with Typescript 3.5.1 release #1307

Closed

xaviergonz added 2 commits July 20, 2019 13:07

merge master, better way to avoid empty objects

4ef4dd4

update dev deps

065112b

xaviergonz requested a review from mweststrate July 20, 2019 11:26

fix unit tests

6b966be

xaviergonz changed the title ~~Fix for empty models / models with all properties set to optional being able to take any value in TypeScript [WIP]~~ Fix for empty models / models with all properties set to optional being able to take any value in TypeScript Jul 22, 2019

k-g-a approved these changes Oct 16, 2019

View reviewed changes

mweststrate approved these changes Oct 17, 2019

View reviewed changes

fix merge conflicts, update deps, fix flow typings

f8bfb31

xaviergonz added 2 commits October 17, 2019 19:15

fix size limit version upgrade

a660052

fix build

8a63d54

mweststrate mentioned this pull request Oct 28, 2019

Typescript: Invalid or missing properties allowed in some cases of SnapshotIn #1373

Closed

xaviergonz merged commit b626061 into master Oct 28, 2019

danielkcz deleted the fix-1268 branch October 28, 2019 21:16

mbest added a commit to mbest/mobx-state-tree that referenced this pull request Apr 14, 2020

Remove castFlowReturn from docs

b21c746

Since mobxjs#1269 marked `castFlowReturn` as deprecated, it shouldn't be mentioned as needed in the docs.

mbest mentioned this pull request Apr 14, 2020

Remove castFlowReturn from docs #1503

Merged

thegedge mentioned this pull request Feb 25, 2024

Eliminate NonEmptyObject #2152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for empty models / models with all properties set to optional being able to take any value in TypeScript #1269

Fix for empty models / models with all properties set to optional being able to take any value in TypeScript #1269

xaviergonz commented Apr 29, 2019 •

edited

Loading

coveralls commented Apr 29, 2019 •

edited

Loading

mweststrate Apr 30, 2019

xaviergonz commented Apr 30, 2019

xaviergonz commented Apr 30, 2019

mweststrate commented Apr 30, 2019 via email

xaviergonz commented May 1, 2019 •

edited

Loading

mweststrate commented May 2, 2019

mweststrate commented Jul 19, 2019

xaviergonz commented Jul 19, 2019

mweststrate commented Jul 19, 2019 via email

danielkcz commented Jul 19, 2019

xaviergonz commented Jul 19, 2019 •

edited

Loading

xaviergonz commented Jul 20, 2019

k-g-a left a comment

danielkcz commented Oct 17, 2019

xaviergonz commented Oct 17, 2019

xaviergonz commented Oct 17, 2019

mweststrate commented Oct 18, 2019

Fix for empty models / models with all properties set to optional being able to take any value in TypeScript #1269

Fix for empty models / models with all properties set to optional being able to take any value in TypeScript #1269

Conversation

xaviergonz commented Apr 29, 2019 • edited Loading

coveralls commented Apr 29, 2019 • edited Loading

mweststrate Apr 30, 2019

Choose a reason for hiding this comment

xaviergonz commented Apr 30, 2019

xaviergonz commented Apr 30, 2019

mweststrate commented Apr 30, 2019 via email

xaviergonz commented May 1, 2019 • edited Loading

mweststrate commented May 2, 2019

mweststrate commented Jul 19, 2019

xaviergonz commented Jul 19, 2019

mweststrate commented Jul 19, 2019 via email

danielkcz commented Jul 19, 2019

xaviergonz commented Jul 19, 2019 • edited Loading

xaviergonz commented Jul 20, 2019

k-g-a left a comment

Choose a reason for hiding this comment

danielkcz commented Oct 17, 2019

xaviergonz commented Oct 17, 2019

xaviergonz commented Oct 17, 2019

mweststrate commented Oct 18, 2019

xaviergonz commented Apr 29, 2019 •

edited

Loading

coveralls commented Apr 29, 2019 •

edited

Loading

xaviergonz commented May 1, 2019 •

edited

Loading

xaviergonz commented Jul 19, 2019 •

edited

Loading