dustingetz / react-cursor

Immutable state for React.js
1.03k stars 50 forks source link

Cursor.apply is broken after introduce update function #49

Closed lijunle closed 9 years ago

lijunle commented 9 years ago

(Fork from #45 )

The update function is introduced in this commit 154b986.

The failing test case details:

1) Cursor should delegate apply method to $apply operation:
     Error: Invariant Violation: update(): expected spec of $apply to be a function; got 8.
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:20641
      at update (file:///home/ubuntu/workspace/test/html/js/spec-runner.js:21908)
      at update (file:///home/ubuntu/workspace/test/html/js/spec-runner.js:21915)
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:22045
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:7544
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:7485
      at ReactCompositeComponent_updateComponent (file:///home/ubuntu/workspace/test/html/js/spec-runner.js:14210)
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:7406
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:15102
      at runBatchedUpdates (file:///home/ubuntu/workspace/test/html/js/spec-runner.js:16926)
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:18712
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:18712
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:16874
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:16948
      at ReactUpdates_flushBatchedUpdates (file:///home/ubuntu/workspace/test/html/js/spec-runner.js:14210)
      at file:///home/ubuntu/workspace/node_modules/es5-shim/es5-shim.js:259
      at :1
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:18785
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:18726
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:9890
      at enqueueUpdate (file:///home/ubuntu/workspace/test/html/js/spec-runner.js:16988)
      at enqueueUpdate (file:///home/ubuntu/workspace/test/html/js/spec-runner.js:16504)
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:16682
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:6620
      at update (file:///home/ubuntu/workspace/test/html/js/spec-runner.js:22056)
      at file:///home/ubuntu/workspace/node_modules/es5-shim/es5-shim.js:259
      at :1
      at file:///home/ubuntu/workspace/test/html/js/spec-runner.js:22212
      at callFn (file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:4562)
      at file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:4555
      at file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:4974
      at file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:5079
      at next (file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:4899)
      at file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:4909
      at next (file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:4844)
      at file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:4876
      at timeslice (file:///home/ubuntu/workspace/node_modules/mocha/mocha.js:6483)
lijunle commented 9 years ago

For this line of code, it constructs the test code to the following object:

React.addons.update(state, {a:{$apply:8}})

Then exception!

Before the check in, it constructs as:

React.addons.update(state, {a:{$apply:function(x) { return x / 8 }}})

So, it seems that, the new updater function API originally can be implemented by $apply function. If we want keep the new API, maybe it could be OK to remove the apply delegation.

Besides, it seems a litter confusing for the following code:

cursor.refine('a').push(function (list) { return list.concat([4,5]) }); // somebody might want to push 4 and 5 to the end of the list, but...

And, one more thing... React do shadow copy to avoid the apply function modify the original object.

E.g, the following code still works fine: (JSFiddle)

cursor.refine('a').apply(function (obj) { obj.value = 10; return obj; }); // will the original obj modified?

How about this? (JSFiddle)

cursor.refine('a').apply(function (obj) { obj.nested.value = 10; return obj; }); // will the original obj modified?

The result seems interesting. :smile:

IMO, it may be better to revert the updater function and leave apply function for users' needs. It has to say that, the apply function is not working well. But, we could better to expose those limitations.

dustingetz commented 9 years ago

I fixed the test https://github.com/dustingetz/react-cursor/commit/d4491fb509a9648a1572a29b9a119e198bad9e5b#diff-f9b3da4de5b1f0fbdd8ab24842ce0412 so i could release this as the current code is stable and react 13 was basically broken until these commits. Leaving this issue open for further thought. I agree that it doesn't look like apply is needed anymore.

lijunle commented 9 years ago

@dustingetz The current update implementation will break the shouldComponentUpdate helper with the following code:

cursor.refine('a').set(function (obj) {
  obj.b = obj.b + 1;
  return obj;
});

I write a failing test case to validate this.


If you do not think it is a bug, it could better to clarify it in README.

dustingetz commented 9 years ago

Yeah, object mutation is not allowed in react-cursor, will clarify the readme, thanks.