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

__source makes component stack less useful #12058

Closed
billyjanitsch opened this issue Jan 20, 2018 · 6 comments · Fixed by #12059
Closed

__source makes component stack less useful #12058

billyjanitsch opened this issue Jan 20, 2018 · 6 comments · Fixed by #12059

Comments

@billyjanitsch
Copy link
Contributor

Do you want to request a feature or report a bug?

Somewhere in between.

What is the current behavior?

When I enable babel-plugin-transform-react-jsx-source to automatically add a __source prop to every component in development, the component stack displayed for certain React warnings changes from displaying the (inferred) name of the rendering component to displaying the filename and line number of the occurrence.

React also has access to the file path, but it is stripped and only the filename is included. This seems to be based on the assumption that the name of a file always maps exactly to the name of the component it exports. In practice, many people place components in ComponentName/index.js, so __source currently makes the stack less useful.

For example, compare:

    in DownloadDropdown (created by Foo)
    in Foo (created by Bar)
    in div (created by Bar)
    in div (created by Bar)
    in div (created by Section)
    in section (created by Section)
    in Section (created by Bar)
    in div (created by App)
    in main (created by App)
    ...

to:

    in DownloadDropdown (at index.js:53)
    in Foo (at index.js:183)
    in div (at index.js:182)
    in div (at index.js:175)
    in div (at index.js:29)
    in section (at index.js:28)
    in Section (at index.js:173)
    in div (at index.js:26)
    in main (at index.js:24)
    ...

What is the expected behavior?

I would like the stack to include the full file path, or at least to include the inferred component name alongside the filename.

Would you accept a PR for either option?

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.2.0. This was introduced in #6771 for 15.2.0.

@gaearon
Copy link
Collaborator

gaearon commented Jan 20, 2018

There is a PR that improves this: #11523.

I'm not sure we'll want to take that particular approach. However maybe we can just add a special case for index.js specifically? I'd feel safer merging that.

@gaearon
Copy link
Collaborator

gaearon commented Jan 20, 2018

i.e. I am proposing that if the filename is index.js, we also include the closest directory name. Seems like this would be sufficient for most practical purposes.

@billyjanitsch
Copy link
Contributor Author

billyjanitsch commented Jan 20, 2018

Sorry for missing #11523! I searched for issues rather than PRs.

What do you think of the approach of removing the common head in #11523? I like that even better that either option I proposed.

Including the closest directory name works too, if not.

Happy to rebase that PR either way.

@gaearon
Copy link
Collaborator

gaearon commented Jan 20, 2018

I’m not sure really. I’m worried this might make the warnings way too long on projects with a lot of folder nesting.

I would prefer that we do the smaller fix first and then maybe look at it again.

@billyjanitsch
Copy link
Contributor Author

billyjanitsch commented Jan 20, 2018

Ok, I will submit a PR for the smaller fix. Thanks for the discussion!

Incidentally, while looking into whether I should enable babel-plugin-transform-react-jsx-self, I couldn't find any uses of __self in the codebase. Maybe it was only used pre-Fiber? Am I mistaken? It gets attached here but is never used.

@gaearon
Copy link
Collaborator

gaearon commented Jan 20, 2018

It’s not used yet. The plan was to use it to see where callback ref owner would be different from a string ref owner. That would be useful if we deprecated string refs. We have a different plan for that now though.

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

Successfully merging a pull request may close this issue.

2 participants