Support Iterators/Iterables, Immutable, other inspector improvements #459

cmmcleod · 2016-12-01T07:39:47Z

Features / Improvements:

Improve inspection/rendering of nested Arrays
Improve inspection of TypedArrays (rendered similarly to Arrays)
Improve display of ArrayBuffers and DataViews (Shows size, no deep inspection)
Iterables are rendered as <constructor name>(…)
Iterables are read only
Iterables entries are rendered as lists/arrays
Inspecting Iterables/Immutable values is greatly improved (but keyed collections need improving. See below)
ES6 Map/Set objects no longer throw an error

Addresses much of Issue #52 and was inspired by PR #149, particularly @gaearon's comment

Code Change Summary:

Moved duplicated getIn(base, path) to separate file /agent/getIn.js
Refactored /agent/dehydrated.js to improve readability (in addition to enhancements)
Added readOnly and uninspectable propteries to meta for dehydrated values where appropriate
/fronted/DataView/DataView.js DataItem Component check for uninspectable when rendering
Added typed_array, array_buffer, data_view, iterator cases to Propval.js and previewCoplex.js in addition to some refactoring for readbility
Added several props to empty elements to test/example to demonstrate changes.

Caveats / For future Improvement

Typed Arrays are set read only in the inspector with these changes - Editing them (in the current released version) would cause them to be converted to objects, but it should be possible to handle this.

Iterators/Iterators are read only for fairly obvious reasons, however it might be reasonable to implement a live-edit feature for built-in iterables (i.e. Map, Set, TypedArray).

Improvement of Keyed iterables/iterators

With these changes the inspector / DataView renders entries of iterators as Arrays of [key, value] arrays i.e. 0: Array[2], 1: Array[2], ... rather than keyA: <value>, keyB: <value>, ....
Determining if an iterable is a keyed iterable is tricky because an iterator entry can be of any value. Simply checking that entries are arrays of length 2 is insufficient because a Set of arrays (see tuplesSet in example/test/target.js) would 'break' this approach. In addition ES6 Map and Immutable.Map allow complex keys and use Same-value-zero equality to determine uniqueness. This means the keys/attribute strings in the inspectors path cannot easily be converted to strings through either toString or JSON.stringify without breaking correctness in some cases.

Requesting Feedback & Help for:

Code style, improvements, etc
UI/UX/Styling/Color tweaks for Iterables, Typed Arrays, etc?
Ideas for handling keyed iterable case
Community assistance in testing with other collection libraries e.g. seamless-immutable, etc

Finally
Substantial unit test coverage is on it's way, I just wanted to get some eyeballs on this first!

cmmcleod · 2016-12-01T07:44:56Z

gaearon · 2016-12-01T15:23:47Z

Flow is failing, could you fix that?

cmmcleod · 2016-12-01T18:18:54Z

Yes, forgot to run typecheck. Most of this is fixed in the latest commits, however I'm getting a peculiar failure now:

$ npm run typecheck

> @ typecheck ..../react-devtools
> flow check

node_modules/invariant/invariant.js.flow:3
  3: declare module.exports: (
                   ^ Unexpected token .


Found 1 error

Running node v6.3.0 and npm 3.10.3. Does invariant need to be added to the .flowconfig ignore list?

$ npm ls invariant

└─┬ babel-core@6.3.21
  └─┬ babel-traverse@6.19.0
    └── invariant@2.2.2

cmmcleod · 2016-12-01T18:33:33Z

Running with node v4.6.2 and npm 2.15.11 (same as TravisCI) works fine

gaperton · 2017-01-04T07:16:44Z

agent/getIn.js

+      }
+      if (typeof obj[Symbol.iterator] === 'function') {
+        // Convert iterable to array and return array[index]
+        return [...obj][attr];


Won't there be n^2 complexity? getIn seems to be called in a loop, and we traversing the container every time it's called.

Will line 25 work with key-value maps, such as decorated records creating properties accessors for its members (so check in line Components displayed as <Unknown ... /> when declaring js variables at the top. #20 will fail)?

getIn was duplicated code moved fro Bridge.js#441 and Agent.js#401 and updated to support iterators. The complexity of the previous implementation hasn't changed (with the exception of spreading the iterable on line 25).

This does work with key-value maps (if they are iterable) with one caveat, in order to support maps where the key is not a primitive the tool must treat the [key, value] entry as an iterable as well resulting in something like this:

myMap: Map(..) 0: [ 0: <complex key A> 1: "some value" ] 1: [ 0: "string key B" 1: "some other value" ] ...

see Improvement of Keyed iterables/iterators in this comment

I hope I'm making sense here.

gaearon · 2017-02-15T02:26:23Z

Determining if an iterable is a keyed iterable is tricky because an iterator entry can be of any value

Can you use iterable.entries === iterator? I think this is what we used in React for a while (we are removing support for it though).

gaearon · 2017-02-15T02:27:06Z

Can you rebase please? Sorry for the trouble. I'd like to give this a review.

cmmcleod · 2017-02-15T05:26:55Z

Thanks for the iterable.entries suggestion. I just noticed this in React the other day infact, too bad it's being removed.

Will update and add tests (this may take a day or two for me to get around to it)

gaearon · 2017-02-15T21:58:36Z

Thanks. Excited about this PR!

gaearon · 2017-02-24T21:25:00Z

Ping, do you think you'll have time to finish this?

cmmcleod · 2017-02-24T21:50:18Z

Yeah, pretty confident I'll have this updated by the end of this weekend!

gaearon · 2017-02-24T21:52:22Z

Awesome.

cmmcleod · 2017-02-27T19:27:56Z

Fixed the conflicts.

Looking into using iterable.entries for Map types

cmmcleod · 2017-03-02T18:18:39Z

objMap: new Map([
    [{ a: 'a'}, true],
    [{ a: 'a'}, false],
    [{ b: 'b'}, [1, 2, 3]],
    [{ c: 'c'}, { k1: 'v1', k2: 'v2' }],
  ])

This looks ok I suppose, but not necessarily the most helpful for dev/debugging since you cannot directly identify the key. However if you are using Objects as keys then presumably you would typically be using other/additional debugging techniques..

gaearon · 2017-03-02T18:22:32Z

How does Chrome display this?

gaearon · 2017-03-02T18:23:56Z

Hmm, maybe we should just do this instead.

What do you think? How hard would this be?

cmmcleod · 2017-03-02T18:38:33Z

Pretty straightforward actually:

gaearon · 2017-03-02T18:42:28Z

Nice! If you could just add the [[Entries]] thing to make it clear what's going on, that would be rad.

cmmcleod · 2017-03-02T18:56:27Z

Cool, have this working now, but I'm just investigating/fixing a bug for nested iterables. Will update once I've fix it.

cmmcleod · 2017-03-02T20:07:47Z

Hmm.. So nesting keyed entries under [[Entries]] has opened up a can of worms because it breaks the way that getIn is expected to work.

I.e. getIn is expected to traverse the 'natural' path, but since we are adding [[Entries]] upon inspection this kind of throws a spanner in the works which means getIn becomes rather hacky and brittle.

I think either the initial implementation (without entries support) or the implementation mentioned in this comment are saner options, but I'm open to suggestions.

gaearon · 2017-03-02T20:09:16Z

OK, let's display it in tuples, like you did originally.

This reverts commit f3c5fe4.

jaredly

this is really well written! Thanks for putting this together. I'm really glad it's able to accommodate e.g. Immutable without adding special cases for it.
I'd like to see us limit array & iterable expansion, but it's not a showstopper

jaredly · 2017-03-08T22:39:02Z

agent/Bridge.js

+      if (val && typeof val[Symbol.iterator] === 'function') {
+        var iterVal = Object.create({});  // flow throws "object literal incompatible with object type"
+        var count = 0;
+        for (const entry of val) {


this will get fairly unhappy for long lists. Can we bail after ~100 and indicate that there's more we're not showing?

this can probably be done as a separate diff, I just would rather an extension update not make people's apps suddenly really slow under debugging

jaredly · 2017-03-08T22:45:11Z

agent/dehydrate.js

+    return 'array_buffer';
+  }
+
+  var type = typeof data;


I think I have a slight preference for doing this first, as all of the above checks are only if typeof -> 'object'

Yeah, that's a good point, easily refactored. Will do

jaredly · 2017-03-08T22:48:31Z

agent/dehydrate.js

+      if (level > 2) {
+        return createDehydrated(type, data, cleaned, path);
+      }
+      return data.map((item, i) => dehydrate(item, cleaned, path.concat([i]), level + 1));


this would also be nice to limit to e.g. 100, but I'm fine w/ just putting a TODO

Adding a fixed limit seems like a reasonable (but temporary) compromise.

I think the more ideal solution would be to add an optional breadth argument (defaulting to some small number (20-30) and then adding a show more... link/button in the interface which would increase the breadth and inspect+render the next 'page' of items (regardless of whether the inspected object is a iterable, array, map, etc).

I'm happy to update the PR with a hardcoded limit and add a TODO

gaearon · 2017-03-08T23:16:27Z

@jaredly Thank you very much for reviewing this.

@cmmcleod Let’s land this after:

Hardcoding a limit on 100 items with a temporary (Only first 100 items were shown) in UI or something like this.
Refactoring the typeof thing.

cmmcleod · 2017-03-08T23:29:10Z

Sounds good. Will get this done and updated tonight

cmmcleod · 2017-03-21T20:32:55Z

Took me a lot longer to get around to this than I would have liked!

Refactored typeof in dehydrate.js
Added hardcoded limit and TODO.

Unfortunately I didn't find any simple mechanism to add (Only first 100 items were shown), but I've been working on a better solution which I'd like to submit in a separate PR (click to load more type of solution).

gaearon · 2017-03-22T23:29:30Z

Could you fix lint please? Something minor:

/home/travis/build/facebook/react-devtools/test/example/target.js
  454:12  error  'i' is already declared in the upper scope  no-shadow
  508:14  error  'i' is already declared in the upper scope  no-shadow

gaearon · 2017-03-27T13:23:47Z

Just a heads up: I'm in the middle of some other work but I'll get back to this as soon as I can. Likely within one or two weeks.

gaearon · 2017-04-19T14:31:22Z

One potentially confusing thing I noticed is it still lets you edit stuff inside the entries (but this won’t work since they’re not real arrays).

gaearon · 2017-04-19T15:08:12Z

Not going to hold this any longer. 😄
This is an incredible PR, thank you so much!

I’ll create a new issue for the followup work.

gaearon · 2017-04-19T15:51:54Z

Please see #608 for followup items.

Let me know if you’d like to work on these! I’d be happy to give you contributor permissions if that would help you move faster.

cmmcleod · 2017-04-21T04:57:17Z

Thanks for the merge! I'm definitely up for looking into some of the remaining issues and will follow up on #608.

facebook-github-bot added the GH Review: review-needed label Dec 1, 2016

facebook-github-bot added the CLA Signed label Dec 1, 2016

gaperton reviewed Jan 4, 2017

View reviewed changes

gaearon mentioned this pull request Feb 15, 2017

Generic support for third-party data objects inspection (ImmutableJS, BackboneJS, etc) #469

Closed

gaearon mentioned this pull request Feb 15, 2017

Tools should display an additional field for the 'native js' version of Immutable objects #52

Closed

gaearon added this to the 2.x milestone Feb 15, 2017

gaearon mentioned this pull request Feb 15, 2017

Cannot expand Sets in state viewer #435

Closed

gaearon mentioned this pull request Feb 20, 2017

Fix #388 : Displaying date appropriately #559

Closed

Colin McLeod added 9 commits February 27, 2017 10:55

pull getIn to seperate file from Agent & Bridge

ec28b8f

Agent uses external getIn

d66d750

Bridge converts iterables to arrays

6c8b5ac

Add support for Typed Arrays, Iterables, ArrayBuffers, DataViews

e65a675

Add test props to test/example to demonstrate inspection of Iterators

56f522f

Bridge converts iterable to object intead of array

033be6e

Fix Flow errors for Bridge.js

eaea710

Fix flow errors for dehydrate

2c18742

Remove flow typechecking from getIn

27f58a2

cmmcleod force-pushed the master branch from 3195fa9 to 27f58a2 Compare February 27, 2017 19:26

Revert "Support entries for iterables".

5d56a17

This reverts commit f3c5fe4.

jaredly approved these changes Mar 8, 2017

View reviewed changes

Colin McLeod added 4 commits March 21, 2017 13:05

Refactor/Optimize dehydrate getProptype

af638f1

Add build script for test/example

1c276ae

Set arbitrarty limit for iteraing large iterables in Bridge

ba8f87f

Add large iterable to test test/example PropTester

da50d3f

Fix linting error in example/target.js

8d9d8dc

gaearon mentioned this pull request Apr 10, 2017

Bugfix: Correctly calculate Overlay position when inspecting a node within iframe(s) #552

Merged

gaearon merged commit 361bc08 into facebook:master Apr 19, 2017

This was referenced Apr 19, 2017

Support for inspecting Immutable.js objects #149

Closed

Followup Work for Iterable Support #608

Closed

gaearon mentioned this pull request Apr 20, 2017

Vigorous perusal of large Redux stores leads to non-responsiveness and eventually OOM #599

Closed

lukeknep mentioned this pull request May 4, 2017

Support Inspection of Immutable JS objects #83

Closed

Support Iterators/Iterables, Immutable, other inspector improvements #459

Support Iterators/Iterables, Immutable, other inspector improvements #459

Conversation

cmmcleod commented Dec 1, 2016 • edited Loading

Features / Improvements:

Code Change Summary:

Caveats / For future Improvement

Improvement of Keyed iterables/iterators

Requesting Feedback & Help for:

cmmcleod commented Dec 1, 2016

gaearon commented Dec 1, 2016

cmmcleod commented Dec 1, 2016 • edited Loading

cmmcleod commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Feb 15, 2017

gaearon commented Feb 15, 2017

cmmcleod commented Feb 15, 2017

gaearon commented Feb 15, 2017

gaearon commented Feb 24, 2017

cmmcleod commented Feb 24, 2017

gaearon commented Feb 24, 2017

cmmcleod commented Feb 27, 2017

cmmcleod commented Mar 2, 2017

gaearon commented Mar 2, 2017

gaearon commented Mar 2, 2017

cmmcleod commented Mar 2, 2017

gaearon commented Mar 2, 2017

cmmcleod commented Mar 2, 2017

cmmcleod commented Mar 2, 2017

gaearon commented Mar 2, 2017

jaredly left a 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

Choose a reason for hiding this comment

gaearon commented Mar 8, 2017

cmmcleod commented Mar 8, 2017

cmmcleod commented Mar 21, 2017

gaearon commented Mar 22, 2017

gaearon commented Mar 27, 2017

gaearon commented Apr 19, 2017

gaearon commented Apr 19, 2017

gaearon commented Apr 19, 2017 • edited Loading

cmmcleod commented Apr 21, 2017

cmmcleod commented Dec 1, 2016 •

edited

Loading

cmmcleod commented Dec 1, 2016 •

edited

Loading

gaearon commented Apr 19, 2017 •

edited

Loading