Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Support for inspecting Immutable.js objects #149

Closed
wants to merge 4 commits into from

Conversation

garetht
Copy link

@garetht garetht commented Aug 16, 2015

This pull request makes changes meant to support the functionality requested for in #52.

Changes Made

  • When an object is an Immutable object, it is now expandable so that its properties can be examined
  • The display name for the object notes that it is an Immutable object
  • Additional tests for the dehydrator and Immutable utilities have been added

@garetht garetht changed the title Immutable Support for inspecting Immutable.js objects Aug 16, 2015
@garetht garetht closed this Aug 16, 2015
@garetht garetht reopened this Aug 16, 2015
@jaredly
Copy link
Contributor

jaredly commented Aug 19, 2015

Hmmm. So that looks reasonable.
My inclination would be to instead implement this as a plugin, and either:
1 - make it so that plugins can alter the representation of props, or
2 - show immutable objects in an extra section in the sidebar, separate from props

Thanks for putting this together!
@spicyj do you have an opinion on how this is implemented?

@jaredly jaredly mentioned this pull request Aug 19, 2015
@garetht
Copy link
Author

garetht commented Aug 19, 2015

That makes sense to me. Do you have a sample plugin I can consult?

return data.constructor.name;
}
},
shallowToJS(dataStructure: any): any {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask your rationale behind providing a shallowToJS here but not a deepToJS that calls Iterable.toJS()? For example, if a particular prop is an Immutable.KeyedIterable with a property value that is itself an Immutable object, then it seems that shallowToJS would not convert the property value to a mutable object.

Edit - I see that your test for this module already asserts the above. Would it not be valuable to do a deep .toJS() call for any immutable object so that the dev tool user can more easily navigate nested immutable properties and state values?

Edit 2 - After digging a bit deeper, it looks like you're doing a shallow conversion at each level so that the user can easily see whether or not some nested property is immutable or not while also being able to easily inspect its child properties. If that's the case, 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just about to write that. The reason in edit 2 is precisely why.

@jackwanders
Copy link

@garetht, I believe @jaredly is referring to the idea of extraPanes, as implemented in frontend/Panel.js. Currently, it looks like the rnStyle capability is used to conditionally add an extra pane for ReactNative styles.

I'm not sure what the most appropriate way to implement these utils as a conditional functionality is, but one option might be to render an ImmutableInspector pane underneath the existing component inspection pane when the user inspects an immutable object. Some kind of top level toggle would be nicer, in my opinion, but may require more work as there doesn't appear to be an existing UI element in which to render something like a checkbox that, when checked, calls .shallowToJS on all immutable objects.

@jackwanders
Copy link

@garetht, @jaredly, I took a pass at implementing the "inspect immutable objects as js" functionality as an optional setting.

garetht#1

This pull request creates a new checkbox at the top of PropState which, when checked, will toggle a property in Store and force an update of Container. The change wound up touching more modules than I had hoped, and someone with a better understanding of the devtools architecture may be able to clean things up. Additionally, I don't believe editing "js-ified" values would work as expected.

@jaredly
Copy link
Contributor

jaredly commented Sep 11, 2015

Yeah -- I'm hesitant to bake in Immutable support; it would be awesome to see this as a plugin; I'll take some time this weekend to figure out what that would look like

@Daniel15
Copy link
Member

👍 I'm looking forward to this!

@zackargyle
Copy link

I decided against using Immutable for this very reason. It really renders the dev tools useless in inspecting data. As long as immutability is being encouraged through projects like redux, I think the few extra lines of code should definitely be baked into the devtools.

@kristian-puccio
Copy link

You can always select an object in the dev tools and query it using $r I
think. Then just use .toJS() in the console to print out the value.

But yeah it's definitely not as quick as it could be

On 15 January 2016 at 04:41, Zack Argyle notifications@github.com wrote:

I decided against using Immutable for this very reason. It really renders
the dev tools useless in inspecting data. As long as immutability is being
encouraged through projects like redux, I think the few extra lines of code
should definitely be baked into the devtools.


Reply to this email directly or view it on GitHub
#149 (comment)
.

@Daniel15
Copy link
Member

@zackargyle - You could always clone @garetht's version of the React devtools (ie. the branch this pull request points to) and try it out :)

@gaearon
Copy link
Contributor

gaearon commented May 19, 2016

As for the feature, I think it is really necessary we get something like this merged.
However I’m not sure about this implementation.

I think we should strive to support any iterables, and not just Immutable.js specifically. As an example, react-json-tree checks whether something has an iterator and handles iterable nodes in a specific way. I think we should do the same here.

Would you like to look into reworking this PR to support arbitrary iterables? Thanks.

@gaearon
Copy link
Contributor

gaearon commented Aug 19, 2016

Closing as there is no response from the author.

@gaearon
Copy link
Contributor

gaearon commented Apr 19, 2017

A more generic support for this landed in #459.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants