Skip to content

fix(ArrayObservation): ensure patch applied only once#704

Merged
EisenbergEffect merged 3 commits intoaurelia:masterfrom
bigopon:fix-patching-array
Aug 1, 2018
Merged

fix(ArrayObservation): ensure patch applied only once#704
EisenbergEffect merged 3 commits intoaurelia:masterfrom
bigopon:fix-patching-array

Conversation

@bigopon
Copy link
Member

@bigopon bigopon commented Aug 1, 2018

Things may still go horribly wrong if one ended up having two versions of aurelia-binding in the final bundle, but at least array patching won't happen twice. Also trim some unnecessary fat out

Edit: this will work as long as there is no difference between v1 and v2 ModifyArrayObserver / ModifyCollectionObserver

@EisenbergEffect Should we also replace global karma start with npm script ?

resolves #699

@3cp
Copy link
Member

3cp commented Aug 1, 2018

Suggest to log an error in console if it detects already patched, explain possible cause (3rd party plugin), and provide a link to suggested solution. Because duplicated aurelia-binding will inevitably introduce weird bug even without duplicated patch on Array.prototype.

@bigopon
Copy link
Member Author

bigopon commented Aug 1, 2018

@huochunpeng nice suggestion 👍

@EisenbergEffect
Copy link
Contributor

I love this idea. One change: could you switch the lookups back to using property access instead of array access? That will probably cause a perf change and since this is a hot code path, I'd rather not change that.

@bigopon
Copy link
Member Author

bigopon commented Aug 1, 2018

@EisenbergEffect done

@EisenbergEffect EisenbergEffect merged commit 46fa606 into aurelia:master Aug 1, 2018
@EisenbergEffect
Copy link
Contributor

Thanks!

@bigopon bigopon deleted the fix-patching-array branch August 1, 2018 03:39
@rmja
Copy link

rmja commented Aug 1, 2018

Just a reminder that this change should be merged back into v1 in a minor or patch release to work properly😁

@fkleuver
Copy link
Member

fkleuver commented Aug 1, 2018

Just a reminder that this change should be merged back into v1 in a minor or patch release to work properly😁
^^^^^^

@EisenbergEffect Did this go into v1 too?

@bigopon nice one! :)

@EisenbergEffect
Copy link
Contributor

working on a 1.7.2 release with this same fix

EisenbergEffect added a commit that referenced this pull request Aug 2, 2018
fix(ArrayObservation): ensure patch applied only once
@EisenbergEffect
Copy link
Contributor

1.7.2 and 2.1.3 both published with this fix.

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.

Add protection agains double registration on Array.prototype.*

5 participants