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

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

Closed
wants to merge 13 commits into from
Closed

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

wants to merge 13 commits into from

Conversation

gaperton
Copy link

@gaperton gaperton commented Jan 5, 2017 •
edited
Loading

General problem is that sometimes developers are using data objects which are more sophisticated than raw JS objects and arrays. Typically, these objects are represented as some facade class at the top level holding an actual state inside. Navigating through such an objects in props as well as in state is clumsy and the property inspector is almost unusable.

There is a number of issues on this topic (#83, #52). And there was an attempt to solve it in the PR specifically for ImmutableJS: #149

In this PR I attempt to solve the problem in the way that it would be easy for data framework developers to customize the appearance of data objects in the inspector. I'm doing it to make NestedReact models and collections look good in the inspector, but it's trivial to do it for any other data library including ImmutableJS (which I can do as well to resolve that pool of duplicate issues as soon as this PR will be accepted).

Idea is to attach state inspector functions to the object's prototype, which will take an object instance and return plain JS object or array (whatever is appropriate for the particular case). Whenever an object has state inspector it will be used to transform its state. Using BackboneJS as an example, something like this needs to be done to make models and collections look good:

if (__REACT_DEVTOOLS_GLOBAL_HOOK__) {
  const { addInnerStateInspector } = __REACT_DEVTOOLS_GLOBAL_HOOK__;
  if (typeof addInnerStateInspector === 'function') {
    addInnerStateInspector( Backbone.Model, x => x.attributes );
    addInnerStateInspector( Backbone.Collection, x => x.models );
  }
}

Another example - Date instances currently appears as an empty object (#388). Which is fixed by this PR in the following way:

addInnerStateInspector( Date, ( x : Date ) => ({
   local : `${ x.toDateString() } ${ x.toTimeString() }`,
   iso : x.toISOString(),
   timestamp : x.getTime(),
});

This API can be used internally to improve the appearance of built-in JS classes, by data framework developers to make their custom classes appear well, and by regular developers as well.

In the case of the more sophisticated data structure, the corresponding raw JS representation can be calculated inside of the inspector functions and memorized in the data object's instance (which will work perfectly in case of ImmutableJS).

P.S.: One more issue this PR resolves (#390) is an exception being thrown when you're trying to inspect an object with get prop(){...} calculated properties defined on the prototype.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@gaperton
Copy link
Author

gaperton commented Jan 5, 2017 •
edited
Loading

If this PR will be accepted, I will implement support for ImmutableJS data structures as well. Not that big deal.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaperton gaperton changed the title Generic Support for third-party data objects instection (ImmutableJS, BackboneJS, etc) Generic support for third-party data objects inspection (ImmutableJS, BackboneJS, etc) Jan 5, 2017
@zavelevsky
Copy link

+1 big time. Can save a lot of hassle.

@wizzard0
Copy link

wizzard0 commented Jan 5, 2017

+1 too, integration is important

@elektronik2k5
Copy link

elektronik2k5 commented Jan 5, 2017 •
edited
Loading

I like the idea, but not the magic property _innerState. I suggest to replace it with a framework specific function (implemented inside each framework), which would register itself with the devtools (via a global, when devtools is active) and implement this logic on it's own: both the identification of whether a given object has this 'inner state', and what should it return.

Disclaimer: I don't really know how react devtools hooks work, so this is more of a pseudo-code than something usable:

// inside Backbone:
if (__REACT_DEVTOOLS_GLOBAL_HOOK__) {
  const { addInnerStateInspector, } = __REACT_DEVTOOLS_GLOBAL_HOOK__
  if (typeof addInnerStateInspector === 'function') {
    addInnerStateInspector(function inspectBackboneModel(possiblyModel){
      // is this a Backbone Model?
      const isInspectable = possiblyModel instanceof Backbone.Model
      return {
        isInspectable,
        value: isInspectable ? possiblyModel.attributes : undefined, // this is the value to show
      }
    })
    addInnerStateInspector(function inspectBackboneCollection(possiblyCollection){
      // is this a Backbone Collection?
      const isInspectable = possiblyCollection instanceof Backbone.Collection
      return {
        isInspectable,
        value: isInspectable ? possiblyCollection.models : undefined, // this is the value to show
      }
    })
  }
}

@gaperton
Copy link
Author

gaperton commented Jan 5, 2017 •
edited
Loading

I suggest to replace it with a framework specific function

This code below behaves exactly in the same way, works faster, and framework developer doesn't need to know anything specific about dev tools hooks.

if (__REACT_DEVTOOLS_GLOBAL_HOOK__) {
    Object.defineProperty( Backbone.Model.prototype, '_innerState', {
        get : function(){
            return this.attributes; // That's internal key-value hash.
        }
    });

    Object.defineProperty( Backbone.Collection.prototype, '_innerState', {
        get : function(){
            return this.models; // That's an array of models
        }
    });
}

The decision, whenever it should or shouldn't be guarded with REACT_DEVTOOLS_GLOBAL_HOOK check, is entirely on framework developer. In the real framework it looks like a one-liner, get _innerState(){ return this.attributes; } ), and I see no point to guard it with any checks.

So, what exactly is the benefit in making it more complex?

@elektronik2k5
Copy link

@gaperton the benefit is prevention of false positives. Any object can have a property called _innerState - in which case the devtools would lie about that object's contents. Any convention is bound to break at some point - it's only a matter of probability, and while rare, such a case would result in a terrible situation, which is basically impossible to debug. Who would assume that a devtool is buggy? It would be literally the very last place I'd check (if at all) - past the point of despair and well into insanity o_O

My solution guarantees interop and prevents accidental false positives. You want to have that kind of certainty in order for your tools to be reliable.

@iddan
Copy link

iddan commented Jan 5, 2017

👍

@gaperton
Copy link
Author

gaperton commented Jan 5, 2017

@elektronik2k5 I don't think that such a dramatic increase of the complexity as in your proposal (not to mention that it will work dramatically slower) worth potential benefits of avoiding the risk of occasionally overlapping name in the class member. As there are way cheaper ways to mitigate such a risk. For instance, rename the property _innerState to something more specific, such as __REACT_DEVTOOLS_EXPOSED_STATE_YOU_KIDDING_ME_CLAIMING_IT_WILL_OVERLAP__.

My solution guarantees interop and prevents accidental false positives. You want to have that kind of certainty in order for your tools to be reliable.

I don't see the solution yet. Just the proposal. But you're welcome to proof its superiority by actually implementing it and creating the PR.

@gaperton
Copy link
Author

gaperton commented Jan 5, 2017

For instance, rename the property _innerState to something more specific

That's a good idea, in fact. __inner_state__ should be practically safe enough.

@gaperton
Copy link
Author

gaperton commented Jan 6, 2017 •
edited
Loading

@elektronik2k5 From another hand, why not. Just changed an API a bit, so it actually looks much nicer than the usage of raw __inner_state__. Now it looks like something that civilians can use.

Updated PR description.

// inside Backbone:
if (__REACT_DEVTOOLS_GLOBAL_HOOK__) {
  const { addInnerStateInspector } = __REACT_DEVTOOLS_GLOBAL_HOOK__;
  if (typeof addInnerStateInspector === 'function') {
    addInnerStateInspector( Backbone.Model, x => x.attributes );
    addInnerStateInspector( Backbone.Collection, x => x.models );
  }
}

This was referenced Jan 6, 2017
Closed
Closed
@gaperton gaperton mentioned this pull request Jan 6, 2017
Closed
@wmill
Copy link

wmill commented Jan 13, 2017

+1

@gaperton
Copy link
Author

Guys, please, respond. We need these three issues to be fixed.

@gaearon
Copy link
Contributor

gaearon commented Jan 27, 2017

@gaperton

React team is currently busy with rewriting React itself (facebook/react#7925). We recognize it's annoying but we just don't have the capacity to review big changes right now.

If this was implemented as support built in on existing JS primitives (e.g. iterables) instead of adding APIs it would be easier to review. Most people who need Immutable (or other) support wouldn't guess that they need to also install a special hook for it to work. So I don't think it's a very good long term approach.

@gaperton
Copy link
Author

gaperton commented Jan 28, 2017 via email

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2017

people who need Immutable support will just see Immutable stuff
working. They don't need to install any hooks. That API is for framework
developers.

Maybe I misunderstood you, but is there a corresponding PR for Immutable that has been reviewed and approved by its maintainers? I'm open to merging this if there is a broad agreement on the hooks between library maintainers but I haven't seen this yet.

@gaperton
Copy link
Author

gaperton commented Jan 28, 2017 via email

@gaperton
Copy link
Author

gaperton commented Jan 28, 2017 via email

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2017

Just to remind you - currently Date shown as empty object

Could you send a fix for that in particular? It shouldn’t be hard to detect a native Date.

attempt to access objects with "get" properties throws exceptions

Why? Could we also fix this by doing whatever Chrome DevTools is doing for them? It doesn’t seem like this would require custom hooks either.

I will implement support for ImmutableJS if this PR will be accepted

The reason I’m asking is it is much harder to get support from libraries than from us. I would like to see Immutable maintainers to at least agree to an RFC proposal (you don't have to send a PR) before proceeding with this. Since you approach this as a generic solution that requires library opt-in it makes sense to first get some feedback and commitment from library authors on the API.

If you think Immutable maintainers won't accept this PR unless we also merge its support, doesn't it raise any issues about the approach itself?

@gaperton
Copy link
Author

gaperton commented Jan 28, 2017 •
edited
Loading

Could you send a fix for that in particular? It shouldn’t be hard to detect a native Date.

The fix for Date currently relies on addInnerStateInspector API. Of course, it can be hardcoded with all related type checks (and the solution size will be the same).

While it can be done, I don't understand why do you think it's beneficial to rewrite the code in the way that it would be hard to do the same thing for any other data type. Including RegExp, byte arrays, files, etc. What exactly is the benefit in not exposing an API which make the fix for Date looks trivial?

@gaperton
Copy link
Author

gaperton commented Jan 28, 2017 •
edited
Loading

attempt to access objects with "get" properties throws exceptions

Why?

Good question, Dan. Here's the ticket about it. #390

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2017

Good question, Dan. Here's the ticket about it. #390

Seems like it has a proposed solution: #390 (comment).

The fix for Date currently relies on addInnerStateInspector API. Of course, it can be hardcoded with all related type checks (and the solution size will be the same).

Right—the difference is that unlike the solution with a new API, it's easy to merge because we don't need to agree on the API, only on the implementation.

I don't understand why do you think it's beneficial to rewrite the code in the way that it would be hard to do the same thing for any other data type. Including RegExp, byte arrays, files, etc.

Once you add an API you can't easily break it as people come to rely on it. I'm not convinced this is the best API, or even that a public-facing API for injecting monitors is desirable. I agree though that native JavaScript objects that commonly appear as props should be rendered well.

@gaperton
Copy link
Author

The reason I’m asking is it is much harder to get support from libraries than from us. I would like to see Immutable maintainers to at least agree to an RFC proposal (you don't have to send a PR) before proceeding with this. Since you approach this as a generic solution that requires library opt-in it makes sense to first get some feedback and commitment from library authors on the API.
If you think Immutable maintainers won't accept this PR unless we also merge its support, doesn't it raise any issues about the approach itself?

Dan, I have no idea will they accept it or not. Regarding ImmutableJS, as I said - I will implement support for it and create the corresponding PR. If the PR we are discussing will be accepted.

Being framework developer myself, I'm trying to make developer's life better. And I see absolutely no harm, even potential one, in adding experimental API to the dev hook. Even if ImmutableJS maintainers will decide to reject my PR fixing an appearance in Dev Tools, what wrong will happen? This PR is +75/-8 lines of code or so, what are we talking about? Is an idea to expose the hook bad by itself? Why? Will it stop anyone to add inspections for iterables? No, it's a good idea as well. So what?

@gaperton
Copy link
Author

gaperton commented Jan 28, 2017 via email •
edited
Loading

@gaearon
Copy link
Contributor

gaearon commented Feb 15, 2017

I think that it is extremely unlikely a library like Backbone or Immutable would add React-specific hook support. For this reason, this change won't benefit most users anyway.

I also think that the implementation is a bit hacky (e.g. what is __inner_state__? why are we hardcoding a special case for this prop name? what if somebody has a legit use case for it?)

Hacky code is fine if it's worth it, but this just seems to adds complexity that we need to understand and maintain in the future, for little gain, as only a tiny portion of our users will know about the hooks (since, as I said earlier, I don't believe either Backbone or Immutable will add such hooks, based on my experience with open source).

A generic solution like #459 seems like a better way to achieve these goals.

I’m sorry I couldn’t help more.

I’m still happy to take PRs fixing specific cases without exposing new APIs (e.g. to fix Dates).

@gaearon gaearon closed this Feb 15, 2017
@gaperton
Copy link
Author

gaperton commented Feb 15, 2017 •
edited
Loading

I’m sorry I couldn’t help more.

I'm sorry, but it's me who couldn't help more. Given the quality of the discussion here (where you practically avoiding discussing the matter of the topic and ignoring answers), it's highly unlikely that I will spend any more time contributing to the Dev Tools and any other of your repos.

Take care.

@gaearon
Copy link
Contributor

gaearon commented Feb 15, 2017 •
edited
Loading

where you practically avoiding discussing the matter of the topic and ignoring answers

I’m not sure I fully understand what you are referring to. It seems like this PR would only be useful if you could persuade maintainers of Immutable or Backbone to add such hooks. I’m not avoiding anything, I’m just claiming this won’t happen, and you haven’t given any arguments to counter this.

I’ve never seen Immutable/Backbone/etc adding view library-specific hooks, and React is not in some sort of unique position to dictate such integrations. What about Angular, Ember, Vue, Preact, etc? It doesn’t make sense for Immutable or Backbone authors to make special concessions for React. If you think it does please explain how this will happen. Talk to those maintainers, invite them to this discussion, demonstrate that you can coordinate this effort. It is your proposal after all. I don’t want to merge something without at least a basic plan, which you have not provided.

Your proposal is basically “let’s merge and see what happens”. My answer is that this won’t work—not because I’m avoiding or ignoring you, but because it rests on unrealistic assumptions. I explained why they are unrealistic right above. If you don’t agree please let me know via counter-arguments rather than via assuming bad faith behavior on my end. Thanks!

@benjamingr
Copy link

@gaearon - @gaperton is a known problematic contributor and we've had issues with him before in other projects. The group of people who commented are all from the same Facebook group where he ranted about it - and you're being the responsible adult here.

I know you know this - but I just thought it's worth to have someone else say this out loud. Rock on.

@gaperton
Copy link
Author

gaperton commented Feb 15, 2017 •
edited
Loading

is a known problematic contributor and we've had issues with him before in other projects

Oh, yes, I am :). I don't remember we ever intersected in any project, Benjamin, but I do perfectly remember that I blocked you on FB for continuous personal insults :). Let's fix it on github as well.

@gaearon
Copy link
Contributor

gaearon commented Feb 15, 2017

Going to lock this thread as it doesn’t seem very productive.
I believe I mentioned the possible follow up items:

  • If you’re confident with the approach in this PR, please demonstrate commitment from third party projects you’re targeting
  • Feel free to send PRs to send specific cases (e.g. Dates)
  • Let’s not attack each other or assume bad intentions

Cheers!

@facebook facebook locked and limited conversation to collaborators Feb 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed GH Review: review-needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@gaperton @facebook-github-bot @zavelevsky @wizzard0 @elektronik2k5 @iddan @wmill @gaearon @benjamingr