Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add applyPatch test case for reference array #186

Merged
merged 3 commits into from
Jun 13, 2017

Conversation

cpunion
Copy link
Contributor

@cpunion cpunion commented Jun 12, 2017

See #185

this.hovers.push(item)
},
removeHover(item) {
this.hovers = this.hovers.filter(i => i.id === item.id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: same as this.hovers.remove(item)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For removeHover, should be !==, or use remove

@cpunion cpunion force-pushed the apply-patch-reference-array branch from 9d8ea73 to 905ee6a Compare June 12, 2017 16:04
@mattiamanzati
Copy link
Contributor

Ok, I think I got where the problem is, basically after the item is removed from the array, dehanceValues is called and calls getValue over the reference, that has'nt got yet an identifier cache.
We could have 2 solutions; one is to remove somehow that call, and the other one is make by default the actual type of reference(T) as T | null where null is when the reference is'nt resolvable due to being removed from the tree, etc..

@cpunion
Copy link
Contributor Author

cpunion commented Jun 13, 2017

Found another place, just called applySnapshot, the stack trace is:

The error was thrown at: 
    at ReferenceType.getValue
    at Node.getValue
    at Node.unbox
    at Array.map
    at ObservableArrayAdministration.dehanceValues
    at ObservableArrayAdministration.spliceWithArray
    at ObservableArray.replace
    at Node.pseudoAction
    at ArrayType.applySnapshot
    at executeAction
    at ArrayType.res
    at Node.applySnapshot
    at ArrayType.ComplexType.reconcile
    at ValueProperty.willChange
    at ObjectType.willChange
    at Array.<anonymous>
    at interceptChange
    at setPropertyValue
    at Project.set
    at ValueProperty.deserialize
    at Node.pseudoAction
    at ObjectType.applySnapshot
    at executeAction
    at ObjectType.res
    at Node.applySnapshot
    at applySnapshot

@mattiamanzati
Copy link
Contributor

@cpunion Thanks again for reporting and for your test! Can you please update this branch from master, and add an example fail case for applySnapshot as separate test? :)

Thanks again! :)

@cpunion
Copy link
Contributor Author

cpunion commented Jun 13, 2017

Sure, I will try. :)

@cpunion cpunion force-pushed the apply-patch-reference-array branch from b8e35d2 to 51868e7 Compare June 13, 2017 15:56
@cpunion
Copy link
Contributor Author

cpunion commented Jun 13, 2017

Added a test case, and rebased on master.

@cpunion cpunion force-pushed the apply-patch-reference-array branch from 51868e7 to 566e4e1 Compare June 13, 2017 16:06
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 94.848% when pulling 566e4e1 on cpunion:apply-patch-reference-array into 1bbd974 on mobxjs:master.

@mattiamanzati
Copy link
Contributor

mattiamanzati commented Jun 13, 2017

Nice! Seems to be fixed! Thanks for your help @cpunion! :) (will be out soon with another fix :P)

@cpunion
Copy link
Contributor Author

cpunion commented Jun 13, 2017

Yeah! Thanks!

Please cut a release today, I was blocked. :)

@mattiamanzati mattiamanzati merged commit d123c4d into mobxjs:master Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants