search

Palindrom / JSONPatcherProxy

ES6 proxy powered JSON Object observer that emits JSON patches when changes occur to your object tree.
94 stars 14 forks source link

Reflect before invoking the callback #22

Closed alshakero closed 6 years ago

alshakero commented 6 years ago

Fixes: https://github.com/Palindrom/JSONPatcherProxy/issues/21

@warpech can you review please?

Reviewing while ignoring the whitespace saves you a minute or two (https://github.com/Palindrom/JSONPatcherProxy/compare/Reflect-earlier?expand=1&w=1)

warpech commented 6 years ago

I will review.

  1. @alshakero I suppose the fix means that you fully agree with my claim that the callback should be called after the reflection?
  2. @alshakero can you think of any risks involved with this change?
  3. @miyconst may we include this in 2.3.1? I think we should
alshakero commented 6 years ago

@alshakero I suppose the fix means that you fully agree with my claim that the callback should be called after the reflection?

I saw this as the desired behavior all along, it was surprising to me that this bug survived until now. JSONPatcherProxy IMO is meant to be a non-intrusive observer. Not an intercepter that gives the chance to throw errors/validate changes before reflection. I agree that reflection should happen before notification.

miyconst commented 6 years ago

@miyconst may we include this in 2.3.1? I think we should

Yes, we can and should.

alshakero commented 6 years ago

There is a lot of room for more DRYness in setTrap. On it.