Types.union not being re-inferred in reassignment

[Daymannovaes]

Bug report

• [x] I've checked documentation and searched for existing issues
• [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 https://codesandbox.io/s/mobx-state-tree-desambiguation-u458s?file=/index.js

Describe the expected behavior The correct type of a union should be inferred every time a new assignment occurs, even if the union doesn't provide a dispatcher.

Describe the observed behavior The type is inferred correctly in the creation but isn't re-inferred in the assignment (unless the union provides a dispatcher), causing an incorrect type assignment.

[Bnaya]

I'm not sure if i understand the issue, When no dispatcher is provided, mst tries to "guess" the model type: https://github.com/mobxjs/mobx-state-tree/blob/19012c2c/packages/mobx-state-tree/src/types/utility-types/union.ts#L93-L112

And by the console logs in your demo, the guess is consistent.

[Daymannovaes]

I don't think the guess is correct. Look, if I set the jobTitle in the .create method, it resolves as Professional, but if I set with an assignment, it resolves to Patient.

Why they would resolve to different types if the content is the same?

I updated the code sandbox to reflect this new information.

[philllies]

The snapshot you use for creating and assigning is not the same. For create you pass in

{
    jobTitle: "physician"
}

For the assignment you use:

{relatedProfessionals: [], jobTitle: "physician"}

Hence the two different outcomes.

[ada-waffles]

I believe I am also running into this issue currently, and I think I can shed a bit more light on the behavior.

I believe the issue comes down to the following chunk of code: https://github.com/mobxjs/mobx-state-tree/blob/666dabd60a7fb87faf83d177c14f516481b5f141/packages/mobx-state-tree/src/types/utility-types/union.ts#L102-L107

This code seems to be prioritizing the type of the current value when reassigning to a union type. The means that, in an eager union (the default), where the expectation is that types are tried in order, once a broader type later in the union has matched a value, narrower types before it won't match again.

The easiest way to demonstrate this is with the following code:

it("should properly process assigned values when the current value is a broader type", () => {
    //Define a type with a snapshot that looks different from the target type to make it clear
    //when the type is being applied properly
    const InnerTest = t.custom<{foo: number}, {foo: string}>({
        name: "InnerTest",
        fromSnapshot: ({foo}) => ({foo: foo.toString()}),
        toSnapshot: ({foo}) => ({foo: parseFloat(foo)}),
        isTargetType: (snapshot: unknown) =>
            !!snapshot && typeof (snapshot as {foo?: unknown}).foo === "string",
        getValidationMessage: (snapshot: unknown) =>
            !snapshot || typeof (snapshot as {foo?: unknown}).foo !== "number" ? "foo must be a number" : ""
    });

    const TestModel = t.model({
        //Create a union with a very narrow type, InnerTest, and frozen<any> at the end as a catch-all
        inner: t.union(t.null, InnerTest, t.frozen<any>())
    });

    //Initialize with the narrower record type
    const test = TestModel.create({inner: {foo: 1}});
    unprotect(test); //Unprotect for conciseness in testing

    //The InnerTest conversion should have taken place
    expect(test.inner).toEqual({foo: "1"});

    //Assign the literal value and then reassign an InnerTest value
    test.inner = null;
    test.inner = {foo: 2};

    //The InnerTest conversion should have still taken place
    expect(test.inner).toEqual({foo: "2"});

    //Assign a value that will be captured by frozen<any> and then reassign an InnerTest value
    test.inner = {bar: "bar"};
    test.inner = {foo: 3};

    //The object will now be treated as a frozen<any> and foo will be a number
    expect(test.inner).toEqual({foo: "3"});
});

Using a custom dispatcher works around this, because the presence of a custom dispatcher overrides the logic linked to above: https://github.com/mobxjs/mobx-state-tree/blob/666dabd60a7fb87faf83d177c14f516481b5f141/packages/mobx-state-tree/src/types/utility-types/union.ts#L97-L100

Since the code I linked to is commented as being in response to a previous issue (#1045), I don't feel I have enough breadth of knowledge here to recommend a fix or submit a PR.

[coolsoftwaretyler]

Hey @Daymannovaes - I think @ada-waffles' recommendation of a custom dispatcher is an excellent one, but I do believe this is either a bug, or maybe more specifically, something we could change to enhance the library. I'm going to label it as such.

Like @ada-waffles - I'm not sure what the fix is, but I would be happy to work with anyone who's interested in discussing it and working on it. One way or another, I'll categorize this and hopefully that'll help us figure out some good ways forward with MST in general.

Thanks for all the help, everyone.