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

Links to component class #5

Closed
vslinko opened this issue Sep 29, 2015 · 31 comments
Closed

Links to component class #5

vslinko opened this issue Sep 29, 2015 · 31 comments

Comments

@vslinko
Copy link
Contributor

vslinko commented Sep 29, 2015

Now component-inspector shows me only component instance location:
Alt text

How to enable all features from this gif?

@lahmatiy
Copy link
Owner

Looks something changed in react and class resolving doesn't work (https://github.com/lahmatiy/component-inspector/blob/master/src/react/inspector/template-info/index.js#L73)
What is your React version?

@vslinko
Copy link
Contributor Author

vslinko commented Sep 29, 2015

0.14.0-rc1

@lahmatiy
Copy link
Owner

Ok. I'll check it.
Also I see no code snippets in popup :(

Coming back to work on it later this week. Fixing issues as well.

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 1, 2015

I checked on 0.14.0-rc1 – it's works as expected. No link to class it means no meta information was attached to class.
Looks like problem with instrumenting here. Could you provide example of class definition you use?

@vslinko
Copy link
Contributor Author

vslinko commented Oct 1, 2015

I'm using es6 classes. Also I'm using a lot of component decorators. Some of my components have 4 or 5 higher order components on them attached via decorators.

import React, {PropTypes} from 'react';
import testable from 'frontend/utils/testable';

@testable()
export default class SignInForm extends React.Component {
  static propTypes = {
    // ...
  }

  render() {
    return (
      <form>
       {/* ... */}
      </form>
    );
  }
}

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 2, 2015

Still works to me :-/
Could you provide instrumented version of your example?
What version of babel and babel-plugin-source-wrapper? Is babel-plugin-source-wrapper goes before any other plugins (except babel-plugin-react-display-name)?

Also you can find an universal version in master, as I promised. All react specifics could be found in https://github.com/lahmatiy/component-inspector/blob/master/src/api-adapter/react.js – maybe some mistake here (but I believe it's an instrumentation problem).

@vslinko
Copy link
Contributor Author

vslinko commented Oct 2, 2015

Variant 1: Class found, render method found, problem in source code.

export default class Test extends React.Component {
  render() {
    return <div>qwe</div>
  }
}

Alt text

Variant 2: Class and render method are found, but they are referenced to wrapped class. Problem in source code still exists.

function decorator(Component) {
  return class Wrapper extends React.Component {
    render() {
      return <Component />
    }
  }
}

@decorator
export default class Test extends React.Component {
  render() {
    return <div>qwe</div>
  }
}

Variant 3 (decorator from node_modules): class not found.

import {connect} from 'react-redux';

@connect()
export default class Test extends React.Component {
  render() {
    return <div>qwe</div>
  }
}

@vslinko
Copy link
Contributor Author

vslinko commented Oct 2, 2015

So, class not found because inspector looking for a wrapped class, but that wrapped class doesn't instrumented.

@vslinko
Copy link
Contributor Author

vslinko commented Oct 2, 2015

Why source code contains html? I don't know.

I'm using:

    "component-inspector": "^1.1.0",
    "express-open-in-editor": "^1.0.0",
    "babel-plugin-source-wrapper": "^1.1.1",

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 2, 2015

What about babel version? I know resulting code for classes was changed some versions ago.

Why source code contains html? I don't know.

It tries get original source by request to server. Requested file is showing in popup header (/src/long/path/to/LocaleButtonsContainer.js on your screenshot). I suppose server response with html instead of original file.

I think we should find way to tell inspector to use bundle source (with its source map) if possible.

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 2, 2015

Thank you for your helpful examples.
Variants 2 and 3 are requiring investigation and improvements. I didn't know decorators could replace class it applied to for new one. Looks like a hack still :)

@vslinko
Copy link
Contributor Author

vslinko commented Oct 2, 2015

What about babel version?

    "babel": "^5.8.23",

I suppose server response with html instead of original file.

Yes, you're right, it works after I configured file serving. But I don't like this.

Looks like a hack still

It's common practice in React ecosystem. Those classes called Higher Order Components like higher order functions.

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 2, 2015

As I realised general problem is caused using decorators (HOC actually).
Problem #3 is exists because connect creates new class in react-redux and I think this code isn't instrumented. I suppose only user code instrumenting now. Probably problem will gone when HOC will be processed as expected.

Yes, you're right, it works after I configured file serving. But I don't like this.

Initial solution was designed for basis.js, where no bundle in dev mode. And module subsytem has source of any executed module, as evaluate it on fly.
But I think we could improve solution here. Some ideas:

  • scan document searching for any scripts, fetch those files and build modules map if possible. Ideally move it to Web Worker (or even to Service Worker in future). But additional requests to files are still required.
  • middleware for dev-server (express or whatever), that serving source fragments by request. Maybe express-open-in-editor may be extended, or new middleware that would provide all necessary functionality.

Second one looks better.

@vslinko
Copy link
Contributor Author

vslinko commented Oct 2, 2015

Probably problem will gone when HOC will be processed as expected.

Problem is gone, but there are two more issues:

  1. Processing vendor code using Babel is too slow.
  2. Class link is still referenced to wrapper component.

Second one looks better.

I agree.

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 2, 2015

Problem is gone, but there are two more issues

It seems we have a misunderstanding. Problem with HOC is not solved yet (but just disscused with colleagues how to fix it). And when it will be fixed, it does not need to instrumenting libraries (in most cases).

I agree.

I'm going to implement new middleware for code fragment extracting. And one more that mix two middlewares (new one and express-open-in-editor) to simplify setup.

@vslinko
Copy link
Contributor Author

vslinko commented Oct 2, 2015

Great news, I'll wait.

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 6, 2015

Mostly done on decorators. It requires new version of babel plugin required (not published yet). Also new custom viewer for details was implemented. Believe you'll like it :)

screen shot 2015-10-06 at 02 49 04

Some edge cases are left to fix. And I'm going to release it (and babel plugin for instrumenting). New express middlewares are coming next.

It would be great if you able to try it on your code base until release.

@vslinko
Copy link
Contributor Author

vslinko commented Oct 6, 2015

Installed babel plugin and inspector from master branches — no decorators support.
Alt text

BTW your screenshot is great!

@vslinko
Copy link
Contributor Author

vslinko commented Oct 6, 2015

Looks like you forgot rebuild inspector.

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 6, 2015

You're right. Fixed.

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 6, 2015

Just notice component name on your screenshot: <Connect(Connect(...(SignInForm)))>. Interesting how does it produced. I believe <SignInForm> should be shown. But probably with decorator support it will be ok.

@vslinko
Copy link
Contributor Author

vslinko commented Oct 6, 2015

Basic decorator support is working:
Alt text

Issues:

  1. render and class is shifted by 1:
    Alt text
    First decorator is showing class of the component, second decorator is showing class of the first decorator.
  2. It works with ES6 decorator syntax, but it doesn't work with classic decoration:
function decorator() {}
class Component {}
export default decorator(Component);

Alt text

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 6, 2015

Thank you for feedback.

  1. It's simple bug, will fix asap.
  2. This issue is more complicated. Since there is no explicit evidences decorator is used. For static analysis it's just a function call. So I have no idea for now how to link wrapped class with class produced by "decorator" and ignore irrelevant cases :-/

@vslinko
Copy link
Contributor Author

vslinko commented Oct 6, 2015

Is it possible to separate those components to different levels?

<Connect(Testable(LocaleButtons))>
  <Testable(LocaleButtons)>
    <LocaleButtons>
    </LocaleButtons>
  </Testable(LocaleButtons)>
</Connect(Testable(LocaleButtons))>

@vslinko
Copy link
Contributor Author

vslinko commented Oct 6, 2015

Like React Devtools do:
Alt text

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 6, 2015

render and class is shifted by 1:

Fixed 4db0260

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 6, 2015

Is it possible to separate those components to different levels?

It's not clear to me. Do you really needs this?

<Connect(Testable(LocaleButtons))>
  <Testable(LocaleButtons)>
    ...
  </Testable(LocaleButtons)>
</Connect(Testable(LocaleButtons))>

I think it's noisy and not necessary. I want improve viewer for cases when wrapper adds extra makrup. But in simple cases virtual component is redundant.
Maybe I missed something. Could you provide more details?

@vslinko
Copy link
Contributor Author

vslinko commented Oct 6, 2015

Do you really needs this?

This is definitely better then Connect(Testable(LocaleButtons)) = React.createClass.
I suggested this as an idea how to handle classic decorators.

@lahmatiy
Copy link
Owner

lahmatiy commented Oct 8, 2015

Hi!
1.2 is published as is.
Can you move your suggestions to new issue? So we could close this issue since this thread has too many q/a.

@vslinko
Copy link
Contributor Author

vslinko commented Oct 8, 2015

done

@vslinko vslinko closed this as completed Oct 8, 2015
@lahmatiy
Copy link
Owner

lahmatiy commented Oct 8, 2015

Thank you a lot!

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

No branches or pull requests

2 participants