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

Route a debug path (parent node path) for use in debugging messages. Use it in one of the warnings #5167

Closed
wants to merge 1 commit into from

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented Oct 13, 2015

Route a debug path (parent node path) for use in debugging messages. Use it in one of the warnings (more to come in the next couple PRs - in particular, the future PR that will solve #140 for us).

@jimfb
Copy link
Contributor Author

jimfb commented Oct 14, 2015

@spicyj @sebmarkbage Ping.

@jimfb
Copy link
Contributor Author

jimfb commented Oct 16, 2015

@spicyj @sebmarkbage Ping.

@sebmarkbage
Copy link
Collaborator

We should pass a pointer. This won't work with re parenting and windowing reuse.

@jimfb
Copy link
Contributor Author

jimfb commented Oct 16, 2015

Ok, updated to use pointers to debug path nodes, which should be easier to update if/when we implement reparenting/windowing. @sebmarkbage @spicyj

…Use it in one of the warnings (more to come).
@facebook-github-bot
Copy link

@jimfb updated the pull request.

@jimfb
Copy link
Contributor Author

jimfb commented Oct 19, 2015

Ping @sebmarkbage @spicyj

@sophiebits
Copy link
Collaborator

Why not just store a pointer to the parent? This seems unnecessarily complicated.

Also, when is the parent path significantly more useful? I had developed something like this originally for the DOM nesting warning and @sebmarkbage convinced me that the owner list is equally useful.

@jimfb
Copy link
Contributor Author

jimfb commented Oct 27, 2015

@spicyj The parent path is most useful when you have a single component that renders to non-trivial markup, and it becomes difficult to figure out exactly which element is firing. Ideally I would give line numbers, but the full parent path is the next best thing.

@sophiebits
Copy link
Collaborator

Maybe we should make line numbers instead then, like #4596.

@jimfb
Copy link
Contributor Author

jimfb commented Oct 27, 2015

I agree, but that's a whole other can of worms. As of today, we don't have line number information and we do have parent information. Once source information is available (ie. we figure out a story around source information, we update the transpilers, etc), I'm happy to help migrate this to line numbers. In the mean time, this is a valuable bit of contextual information that we can provide.

Even if we had line numbers relatively well supported, that relies on the transformer whereas parents are valuable even if you are writing your ReactDOM.createElements by hand.

Anyway, I don't think that should block this diff.

@jimfb
Copy link
Contributor Author

jimfb commented Feb 3, 2016

Replaced by new devtools API.

@jimfb jimfb closed this Feb 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants