Use Proxy.revokable and add revokeProxy to API #4

alshakero · 2017-05-09T14:07:53Z

I'm implementing the needed specs right now.

warpech · 2017-05-09T14:13:01Z

src/jsonpatcherproxy.js

@@ -8,6 +8,7 @@
 /** Class representing a JS Object observer  */
 var JSONPatcherProxy = (function() {
  function JSONPatcherProxy(root) {
+    this.proxifiedObjectsMap = [];


A WeakMap here would be more memory-friendly.

warpech · 2017-05-09T14:17:02Z

src/jsonpatcherproxy.js

-    return proxy;
+    /* keeping track of all the proxies to be able to revoke them later */
+    this.proxifiedObjectsMap.push(proxy);
+    return proxy.proxy;
  };
  //grab tree's leaves one by one, encapsulate them into a proxy and return
  JSONPatcherProxy.prototype._proxifyObjectTreeRecursively = function(


Could _proxifyObjectTreeRecursively automatically revoke proxy of when a child object is removed?

@warpech I started with this yesterday but figured that would render the child object useless even if it was cached earlier on purpose for later usages, like in this demo. Doing it in Palindrom made more sense since Palindrom.obj is ours and we give it to the user for use, so we have the right to destroy parts of it.

But the observed object in JSONPatcherProxies is not ours and I guess we should leave it, and its children, usable.

What do you think?

I see my mistake here. Children will emit patches to the same callback of the parent object even after they are deleted, which is problematic. I think disabling the traps inside _proxifyObjectTreeRecursively would be the safest option.

Great, then let's do it like you say.

@alshakero The jsbin throws 504 to me :| could you send it once again?

Forgive me if I get something wrong. I still don't get why can we revoke on deleteProperty by default.

But the observed object in JSONPatcherProxies is not ours and I guess we should leave it, and its children, usable.

If child proxy object "was cached earlier on purpose for later usages". It's a proxy object, so it's ours and we can destroy it. If user wants to cache his/hers object for later usages after it's unobserved by us, he/she can use originalObject or the object returned by delete/splice - I believe it's the original one https://github.com/Palindrom/JSONPatcherProxy/pull/4/files?w=1#diff-3bfa726203c6cb018f2321c18ca9298eR94 . I'm not sure, @alshakero is it?

@tomalec now I implemented the ability to disable traps for a given proxy, and this happens automatically in deleteProperty trap. It's more fail-proof. And I regret the revoking proposal, it's redundant now. I'll explain:

If an element (say an HTML input field) is bound to a sub-object of the state tree using Polymer binding. And a patch comes to delete that sub-object from the server, editing the field will throw a TypeError (in the time window of Polymer binding async update), and this error will be to the user in production, not to the developer. Any race condition will cause this issue and it will be very hard to reproduce.

WDYT?

@tomalec the whole JSBin is down 🤒

Could you use jsfiddle.net?

Once delete patch is applied binding should be removed as well.
If you are talking about race conditions between browser-to-server and server-to-browser messages, OT should solve it.

@tomalec I don't see any objective argument for revoking against disabling traps. And since I implemented disabling traps, I naturally lean to less redundant work. Do you have any reason to prefer throwing an error rather than covering the edge-case by simply disabling the proxy?

Could we setup a HO call, I feel I miss something in here.

warpech · 2017-05-09T14:17:31Z

src/jsonpatcherproxy.js

+   */
+  JSONPatcherProxy.prototype.revokeProxy = function(proxy) {
+      let deletedIndex;
+      this.proxifiedObjectsMap.forEach(function(el, index) {


With WeakMap this becomes simpler: this.proxifiedObjectsMap.get(proxy).

Totally forgot about its whole existence! Great idea.

And add specs for either case

tomalec · 2017-05-10T10:10:50Z

Just added a comment at https://github.com/Palindrom/JSONPatcherProxy/pull/4/files/a50598b02b1a7c4f34e89afcf5e902defeb83d52#r115702691 but GH hides it.

alshakero · 2017-05-10T10:31:47Z

@tomalec here is my response in case it was hidden: #4 (comment)

- Modify TS typings accordingly - Use `Map` instead of `WeakMap` (Itertable) - Add specs for all new functions and checking for detached warnings - Disable UglifyJS for now (No ES6 Support)

alshakero · 2017-05-15T14:23:55Z

@tomalec I think this is ready for review.

tomalec · 2017-05-15T14:36:51Z

README.md


-#### JSONPatcherProxy#switchCallbackOff () : void
+Enables patches omitting (to both callback and patches array). Starting from the moment you call it.


I would think resume would resume observing/notifying changes, so it would "Disable patches omitting".

What an unlucky typo. I mean emitting.

tomalec · 2017-05-15T17:21:12Z

src/jsonpatcherproxy.js

+      return obj;
+    }
+    var instance = this;
+    var trapsInstance = Object.assign({}, traps, {path: path, instance: instance});


isn't

trapsInstance.path = path; trapsInstance.instance = instance;

cheaper?

Or even just define traps in here, without a need to create 3 new objects?

That's right. I thought not defining the functions (set, get etc..) again and just referencing them would be better. But this would be slower with every issued patch (this.instance.defaultCallback is slow).

Moving them inside could be a little slower but for one time only. Faster with every patch.

I moved it inside.

alshakero · 2017-05-16T13:43:55Z

@tomalec is this good to merge?

tomalec · 2017-05-19T15:06:58Z

README.md


+Turns the proxified object into a forward-proxy object; doesn't emit any patches anymore, like a normal object..


Does .. stands for ellipsis, or it's just one dot too much?

tomalec · 2017-05-19T17:00:24Z

src/jsonpatcherproxy.js

+  /**
+    * Creates an instance of JSONPatcherProxy around your object of interest `root`. 
+    * @param {Object|Array} root - the object you want to wrap
+    * @param {Boolean} showDetachedWarning - whether to log a warning when a detached sub-object is modified @see {@link https://github.com/Palindrom/JSONPatcherProxy#detached-objects} 


WDYT about adding optional parameter info and default values in jsdoc? http://usejsdoc.org/tags-param.html#optional-parameters-and-default-values

tomalec · 2017-05-19T17:18:45Z

src/jsonpatcherproxy.js

+   * Disables all proxies' traps, turning the observed object into a forward-proxy object, like a normal object that you can modify silently.
+   */
+  JSONPatcherProxy.prototype.disableTraps = function() {
+    proxifiedObjectsMap.forEach(disableTrapsForProxy.bind(this));


Why don't we make disableTrapsForProxy instance method as we are always calling it on instance - it results in creating new function of every call, here and on delete? Do you want to keep it private?

tomalec · 2017-05-19T17:21:50Z

test/spec/proxySpec.js

            });

-            observedObj.firstName = 'Malvin';            
-            expect(called).toReallyEqual(1);
+            it('should omit patches when unobserved then observed', function () {


We are testing pause and resume not "unobservered" then "observed"

tomalec · 2017-05-19T17:22:59Z

test/spec/proxySpec.js

            });
+            it('should unobserve then observe again (deep value)', function () {


and we have unobserve again, It's quite confusing to me to mix pause and unobserve test together, under describe('unobserving', function(){

tomalec · 2017-05-19T17:25:12Z

Overall I think it's just cosmetics, but unobserve suite in test is quite confusing to me, wouldn't it be cleaner to have separate section for pause?

alshakero · 2017-05-21T15:30:45Z

@tomalec we have done all kinds of breaking changes, why should we keep unobserve in its current utterly useless case. I removed it and stuck with pause, resume, and disableTraps. And cleaned up the tests.

tomalec · 2017-05-22T21:22:20Z

test/spec/proxySpec.js

+  });
+
+  describe('pausing and resuming', function() {
+    it("shouldn't omit patches when paused", function() {


I think it should be: "Should omit patches when paused"

tomalec · 2017-05-22T21:23:01Z

test/spec/proxySpec.js

+      expect(called).toReallyEqual(2);
+    });
+
+    it('should omit patches when paused then resumed', function() {


I'd say: "Should re-start emitting patches when paused then resumed"

alshakero · 2017-05-23T12:41:02Z

Is this good to go?

alshakero added 2 commits May 9, 2017 15:55

Use Proxy.revokable and add revokeProxy to API

2a402f4

Remove revoked proxies from map

a50598b

warpech reviewed May 9, 2017

View reviewed changes

warpech mentioned this pull request May 9, 2017

Removed objects continue to emit patches when modified Palindrom/Palindrom#134

Closed

warpech requested a review from tomalec May 9, 2017 14:18

alshakero added 3 commits May 9, 2017 17:10

Use WeakMaps and disable traps after remove op

b1022e8

And add specs for either case

Update README

0a27103

Update README - remove old logo attrib

2faa59a

Better API (pause, resume, revoke, disableTraps)

86da839

- Modify TS typings accordingly - Use `Map` instead of `WeakMap` (Itertable) - Add specs for all new functions and checking for detached warnings - Disable UglifyJS for now (No ES6 Support)

tomalec requested changes May 15, 2017

View reviewed changes

alshakero added 3 commits May 16, 2017 09:06

Move traps to local scope, fix README typo

ba14369

Replace all vars with consts

a512ff0

Prettier code

00a3867

tomalec requested changes May 19, 2017

View reviewed changes

Remove unobserve and cleanup tests

e852570

tomalec reviewed May 22, 2017

View reviewed changes

Enhance suites descriptions

5f89c28

tomalec approved these changes May 23, 2017

View reviewed changes

alshakero merged commit f1d8e91 into master May 25, 2017

warpech deleted the Use-Proxy.revokable branch June 5, 2019 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Proxy.revokable and add revokeProxy to API #4

Use Proxy.revokable and add revokeProxy to API #4

alshakero commented May 9, 2017 •

edited by warpech

Loading

warpech May 9, 2017

warpech May 9, 2017

alshakero May 9, 2017 •

edited

Loading

alshakero May 9, 2017 •

edited

Loading

warpech May 9, 2017

tomalec May 10, 2017

alshakero May 10, 2017 •

edited

Loading

alshakero May 10, 2017

tomalec May 10, 2017

alshakero May 10, 2017

tomalec May 10, 2017

warpech May 9, 2017

alshakero May 9, 2017

tomalec commented May 10, 2017

alshakero commented May 10, 2017

alshakero commented May 15, 2017

tomalec May 15, 2017

alshakero May 15, 2017

tomalec May 15, 2017

alshakero May 16, 2017

alshakero commented May 16, 2017

tomalec May 19, 2017

tomalec May 19, 2017

tomalec May 19, 2017

tomalec May 19, 2017

tomalec May 19, 2017

tomalec commented May 19, 2017

alshakero commented May 21, 2017

tomalec May 22, 2017

tomalec May 22, 2017

alshakero commented May 23, 2017 •

edited

Loading


		#### JSONPatcherProxy#switchCallbackOff () : void
		Enables patches omitting (to both callback and patches array). Starting from the moment you call it.


		Turns the proxified object into a forward-proxy object; doesn't emit any patches anymore, like a normal object..

		});
		it('should unobserve then observe again (deep value)', function () {

Use Proxy.revokable and add revokeProxy to API #4

Use Proxy.revokable and add revokeProxy to API #4

Conversation

alshakero commented May 9, 2017 • edited by warpech Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero May 9, 2017 • edited Loading

Choose a reason for hiding this comment

alshakero May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec commented May 10, 2017

alshakero commented May 10, 2017

alshakero commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec commented May 19, 2017

alshakero commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero commented May 23, 2017 • edited Loading

alshakero commented May 9, 2017 •

edited by warpech

Loading

alshakero May 9, 2017 •

edited

Loading

alshakero May 9, 2017 •

edited

Loading

alshakero May 10, 2017 •

edited

Loading

alshakero commented May 23, 2017 •

edited

Loading