mobx-state-tree icon indicating copy to clipboard operation
mobx-state-tree copied to clipboard

Union types differing only by arrays cause subtle downstream bugs

Open davetapley opened this issue 2 years ago • 1 comments

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

This builds on https://github.com/mobxjs/mobx-state-tree/discussions/1906

const Foo = types.model({ foo: types.array(types.string) });
const Bar = types.model({ bar: types.array(types.number) });
const FooBar = types.union(Foo, Bar);

const test_foo = { foo: ["test"] };
const test_bar = { bar: [200] };

Describe the expected behavior

it("validates correctly", () => {
  expect(Foo.is(test_foo)).toBeTruthy();
  expect(Bar.is(test_foo)).toBeFalsy();
  expect(Foo.is(test_bar)).toBeFalsy();
  expect(Bar.is(test_bar)).toBeTruthy();
});

Describe the observed behavior

toBeFalsy expectations fail.


This is how the bug manifested for me:

Sandbox link or minimal reproduction code

const Store = types.model({ foobars: types.array(FooBar) });

const store = Store.create({
  foobars: [test_foo, test_bar],
});

const foo = store.foobars[0];
const bar = store.foobars[1];

Describe the expected behavior

it("gets correctly", () => {
  expect(foo.foo).toEqual(["test"]);
  expect(foo.bar).toBeUndefined();

  expect(bar.bar).toEqual([200]);
  expect(bar.foo).toBeUndefined();
});

Describe the observed behavior

bar.bar is undefined, bar.foo is []


I'm able to work around it buy putting type: types.literal("foo") and type: types.literal("bar") on the models, but it would be nice if MST could let the user know that there are ambiguous union types. (especially when union types which differ by something other than an array seem to work automagically, per https://github.com/mobxjs/mobx-state-tree/discussions/1906)

davetapley avatar May 11 '22 23:05 davetapley

These were the smoking guns for me:

This find picks the first type which appears to match (even if multiple do): https://github.com/mobxjs/mobx-state-tree/blob/cf36cfeedc27d6a0877ccb8b8d3e64169924bcee/packages/mobx-state-tree/src/types/utility-types/union.ts#L108-L110

and, because array is secretly optional, this will return true even if an array property is absent on value: https://github.com/mobxjs/mobx-state-tree/blob/cf36cfeedc27d6a0877ccb8b8d3e64169924bcee/packages/mobx-state-tree/src/types/utility-types/snapshotProcessor.ts#L163

davetapley avatar May 11 '22 23:05 davetapley

@davetapley - really sorry it took so long for anyone to get back to you on this. Do you believe this to be a bug, or do you think we could resolve the issue with better documentation/perhaps a warning in dev mode?

Changing the actual behavior would probably require a breaking change, but I do think we could put together a PR to update the docs and maybe toss a helpful warning about it. What do you think?

coolsoftwaretyler avatar Jun 26 '23 04:06 coolsoftwaretyler