Broken Map type in 5.3.0

[cambirch]

Bug report Upgraded today to 5.3.0 and every type inference pulling Map types from entities suddenly broke.

What I'm doing is I have hook functions that make child maps produce UI. This is via a hook called useField_ChildListEditor. It is passed the parent entity, and the name of the child map to produce UI for.

Previously (and now reverted to 5.2.0) everything was fine. On upgrade it was resolving as an error.

Maybe there is some kind of different TypeScript magic I can use in order to make this all work again (if so I'd be super glad)? Otherwise I guess something rather unexpected slipped in with that really minor map type update in 5.3.0 and I'm not quite sure how to resolve my need to typecheck the property names correctly.

• [x] I've checked documentation and searched for existing issues and discussions
• [x] I've made sure my project is based on the latest MST version
• [x] Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

type SubType<Base, Condition> = Pick<
    Base,
    {
        [Key in keyof Base]: Base[Key] extends Condition ? Key : never;
    }[keyof Base]
>;

export function useField_ChildListEditor<
    TEntity,
    KeyEntity extends keyof SubType<TEntity, Map<string, any>>,
    SubEntity extends TEntity[KeyEntity],
>(entity: TEntity, prop: KeyEntity, { <configuration> }: Props<TEntity, KeyEntity, SubEntity>) {
}

const Model = types.compose("Entity", EntityRoot).props({
   children: types.map(ChildModel),
});

const entity = Model.create({ .... });

const editor = useField_ChildListEditor(entity, "children", { configuration details });

Describe the expected behavior

No type errors.

Describe the error

Argument of type '"children"' is not assignable to parameter of type 'unique symbol'

[chakrihacker]

I will check and update today

[chakrihacker]

Hey @cambirch Can you share more about EntityRoot and ChildModel or share a codesandbox link??

[cambirch]

Absolutely. Had to start from a blank React + TS sandbox since the "official" one didn't want to import the typescript typings. Tried to imitate the official one as much as is reasonable while keeping it as simple as I could.

Error is on line 35 of index.tsx

link to codesandbox issue

[cmdcolin]

@cambirch this is due to maps changing a bunch of stuff from string to string|number in latest. this codesandbox fixes

https://codesandbox.io/s/serene-dan-swh4td

just made this one line change in your code

-  KeyEntity extends keyof SubType<TEntity, Map<string, any>>,
+  KeyEntity extends keyof SubType<TEntity, Map<string|number, any>>,

related PR https://github.com/mobxjs/mobx-state-tree/pull/2079 which i was also questioning https://github.com/mobxjs/mobx-state-tree/pull/2079#issuecomment-1778437641

[cambirch]

Thanks for the quick response. I've got my codebase upgraded and the compiler is no longer failing. I didn't check that specific part of my type definitions because I had figured that was mine and not library level stuff so it shouldn't have been impacted.

I will say that I'm not sure I'm happy with the definition change because it makes it very loose for Map typing and I lose my control of being able to ensure strict enough typing to protect against unintentional bugs. I've now just introduced a significant quantity of potential bugs that typescript won't be detecting anymore.

I would be quite happy if the map type (types.map) was updated to include and <string|number> as available options instead of the current case. Then at least I would be changing a few hundred pieces of code in order to gain extra safety and future upgrade protections.

Anyways, thanks for figuring out the cause, I tried a lot of other cases including importing IMapType and such and nothing was solving.

[coolsoftwaretyler]

Hey @cambirch - I'm sorry to hear the typing changes caused so much friction for you.

I'm going to bring this up at the maintainer meeting for November and see if we can find a middle-ground here. Do you want me to ping you if/when we fix this up for you? Seems like a small change we could release as a patch.

Sorry again for the inconvenience!

[cambirch]

Yes please, if it gets fixed (even if I have to change my type checking again) I would love to know. One of the major reasons we use MST is because it has nice strict pre-type checking which detects a LOT of bugs before it gets out to customers (our testers have caught so many string/number mismatch bugs thanks to MST). So while this particular issue ultimately was a lot smaller than the for instance the month or so it took to re-platform onto MST, having less strictness may allow a few bugs to slip by accident.

I'm completely ok if I need to change types or code to get back the previous limitations (especially if its a pathway to perhaps using typescript to be even more precise in the future). For instance all of our Maps use GUID/UUID keys and I know its technically possible to model that as a special typescript type that is a further subset of string.

As of right now the code is stable and I'm not really worried about things being introduced that may trickle in potential bugs for 2-4 weeks at this point, so I'm not really in a rush for a change, but it would be excellent if it is possible to better control the allowed types of the map so I can get back to string and maybe go further in the future.

Thanks for the response and the quick pointers to fix my problem!

[coolsoftwaretyler]

Hey @cambirch - thanks again for being understanding here. Just got off the maintainer call, and I think @chakrihacker is going to try and find a way where we can support both options here, but it's possible we are going to move forward with the change as-is in 5.3.0, since it sounds like you were able to upgrade to our new types, and the change was in an effort to fix existing bug reports about the types. @chakrihacker said he'll have an update by Friday here.

One way or another, I still want to apologize to you. This is a tricky scenario since we have classified the prior behavior as a "bug" and "incorrect", but since it's been that way for so long, it's reasonable that you (and I imagine others) came to rely on that behavior, so when we released what we thought was a valid minor version, it broke on your end.

The big takeaway outside of this specific type, is that we are going to do some work to make implicit behavior explicit in the future, so we can be more mindful and communicate better about changes. Semantic versioning is only as useful as our own understanding of the existing state of the code base and its usage.

Thanks again, and we'll have one more update for you by the end of the week. Have a good one!