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

devtools 1.1.1 + Immutable.js breaks pure render #88

Closed
davidnorth opened this issue Aug 26, 2015 · 8 comments
Closed

devtools 1.1.1 + Immutable.js breaks pure render #88

davidnorth opened this issue Aug 26, 2015 · 8 comments

Comments

@davidnorth
Copy link

I found that when implementing shouldComponentUpdate, an immutable value in this.props doesn't have the same identity as that value in nextProps when expected. You end up comparing 2 different Immutable maps with the same attributes.

This happens when an action has updated that component's bit of state earlier then some other unrelated action is dispatched, possibly one that does nothing. The component is comparing a reference to that bit of state it had following the first action with the same following the second action. They should have the same identity since the second action's didn't touch that bit of state.

I wrote a failing spec that reproduces the problem. I found that removing devTools caused it to pass.

https://gist.github.com/davidnorth/b6c5991756a7fdaf4d81

Also, I don't know if it's related, but since upgrading to 1.1.1 I no longer see anything of the state in dev tools, only the actions, where previously I saw the raw immutable object representation.

@gaearon
Copy link
Contributor

gaearon commented Aug 26, 2015

Thanks for failing test, best way to start a discussion!

  1. The problem is the same as Slows down with the amount of actions displayed #81: we re-evaluate too eagerly. This isn't hard to fix but I'm not ready to even attempt it before we have some tests, because it's a bit of a tricky area. The best way to contribute is to write tests for devTools.
  2. Actually, 1.1 was supposed to add support for enumerating Immutable stuff (support for iterable structures within state #79). Can I ask you to investigate a bit more why it doesn't work for you?

@davidnorth
Copy link
Author

OK, I'll investigate why I'm not seeing the state.

@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2015

I'm closing until further details.

@gaearon gaearon closed this as completed Sep 3, 2015
@EvanSimpson
Copy link

@gaearon I think I can shed some light on the Immutable part of this conversation.

#79 enabled more convenient rendering of Immutable objects by differentiating them from other 'Object' types and giving them their own node type that could render them without all the Immutable internals. In doing so, it prevented them from being rendered as the top level node in the JSON tree. You can see in the current implementation that only 'Object' and 'Array' types can be root nodes.

In your test case above, @davidnorth, the state being passed as the root node is an Immutable object, and so rootNode will be false and not render anything. I just opened an issue alexkuz/react-json-tree#5 asking if that was intentional, but from the looks of this conversation, probably not so here's the fix I'm currently using.

@chibicode
Copy link
Contributor

@gaearon I just published 0.1.6 for react-json-tree which pulled in @EvanSimpson's changes and I confirmed that an immutable object can be at the root.

@jwdotjs
Copy link

jwdotjs commented Sep 11, 2015

👍 for upgrading react-json-tree to 0.1.6 to fix seeing immutable objects in state, which our team is also affected by

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2015

There's a PR attempted to fix it: #133
Can you please test whether it improves the situation?

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2015

This is fixed in redux-devtools@2.1.5.
Performing an action with DevTools open was O(n), now it's O(1).

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

No branches or pull requests

5 participants