array.shift/pop not working with array length === 1

[tarno0se]

Bug report

Sandbox link or minimal reproduction code
https://codesandbox.io/s/0mn56vlkw0?expanddevtools=1&hidenavigation=1&module=%2Fsrc%2Findex.ts&view=editor

Describe the expected behavior
1 2

Describe the observed behavior
1 ScalarNode$$1 {subpathAtom: Atom$$1, state: 4, hookSubscribers: Object, environment: undefined, subpath: ""…}

[k-g-a]

Thanks! It's definitely a bug. Introduced in 3.2.2. Used to throw setParent is not supposed to be called on scalar nodes at 3.2.1 and lower.

[xaviergonz]

shouldn't it just return the other pushed item?

[k-g-a]

It should return .storedValue (2 in that case) instead of whole ScalarNode object. Will look into this today.

[k-g-a]

At first glance it seems like a mobx bug:
https://github.com/mobxjs/mobx/blob/04dc6200536bb4588ea0ac2af377e426f9955fb0/src/types/observablearray.ts#L157

dehanceValues(values: any[]): any[] {
        // this.value.lenght > 0 check fails
        if (this.dehancer !== undefined && this.values.length > 0)
            return values.map(this.dehancer) as any
        return values
    }

It seems like we should check values.length > 0, not this.values.length. Because this.values is fairly empty at this point: spliceItemsIntoValues() modified it couple of rows before.
@mweststrate , could you confirm? Can make a PR.

[k-g-a]

Made a PR for mobx.
Not sure if we have to do something at MST.
We could upgrade peerDependencies or write a remark at FAQ about this issue?

[xaviergonz]

I think a peer dep upgrade is better, but that's just my personal opinion.

[mweststrate]

Both :) Peer dep is the proper thing to do. But after that, nobody will notice the warnings printed by npm / yarn, as nobody ever looks at them, see needs to be documented as well :-P Op di 11 dec. 2018 om 18:04 schreef Javier Gonzalez < [email protected]>:

…

I think a peer dep upgrade is better, but that's just my personal opinion. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1105 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhKhP5qBWcso0E8aCjgfNB02_EXKdks5u3-WzgaJpZM4ZLdET> .

[k-g-a]

To fix this issue, please upgrade your mobx version to 4.8.0/5.8.0.

[k-g-a]

For new users will be fixed by #1111