mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.94k stars 641 forks source link

Union types differing only by arrays cause subtle downstream bugs #1907

Closed davetapley closed 1 year ago

davetapley commented 2 years ago

Bug report

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 commented 2 years ago

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

coolsoftwaretyler commented 1 year ago

@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?