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

Fix concatenation of null to a warning message #13166

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 7, 2018

Thanks to the renaming in #13165, I noticed a bunch of cases where we could accidentally concatenate null to a warning message if the stack was missing.

I changed getCurrentFiberStackInDevOrNull to always return a string, and renamed it to getCurrentFiberStackInDev. This fixed existing callsites that already relied on it being a string (which wasn't always the case). Then I removed a few || '' that now became unnecessary.

I also changed the "isomorphic" type of ReactDebugCurrentFrame.getStackAddendum() to be tighter since we know both SSR and Fiber implementation return a string now. This is a bit optimistic because in practice ReactDebugCurrentFrame could be from an old React version that didn't have the || '' isomorphic fallback, and its getStackAddendum could be pointing to another renderer that has a version before this change (e.g. maybe something like React 16.4 + React DOM 16.5 + ReactDOMServer 16.4). In that case you could potentially see the null bug in the warning message. I don't think this matters because (a) we recommend to keep versions in sync, (b) we already have this bug in a bunch of places, and I'd prefer tightening it up now than worrying about what happens in an edge case where you mix past and present versions.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 7, 2018

(Why didn't tests catch this? It's not that easy to get no stack at all. You either have to reproduce an existing bug, the kind that #13158 partially fixes, or forget to update the current Fiber pointer, which could also happen if there's a bug.)

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable enough and a nice way to fix the issue. Nice one.

@gaearon gaearon merged commit 96d38d1 into facebook:master Jul 9, 2018
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.

None yet

3 participants