Delimit dynamic part of the warning messages with newlines #8719
Comments
|
Assigning myself so I don’t forget about this issue. I don’t intend to work on this myself but will be happy to review your PR. |
|
I'd like to work on this issue. :) |
|
👍 Please send a PR and ping me! |
|
@gaearon - while browsing through 'codebase-overview.md', I noticed a potential typo in the markdown. On line 116, there appears to be one too many "`" marks, resulting in the rest of the file appearing as code, instead of regular text with code excerpts interspersed when relevant:
I'm happy to fix this as well, but should I create a separate issue for this and submit a PR, or just bundle the fix together with my work on issue #8719? Here's what the markdown would look like with one less "`" symbol:
|
|
Please create a separate PR. |
|
While unrelated, I generally recommend reading docs on the website: |
|
@richiethomas You are working on this issue ? |
|
@AshikNesin yes, I am. :) @gaearon I'd like to make sure I have it right before submitting a PR. As an example of the change being requested, one of the files I'd change would be ReactElementValidator.js, line 36. Looks like I would add 2 newlines at the start of the string that's returned from the 'getDeclarationErrorAddendum' function. In other words, change: ' Check the render method of to this: '\n\n Check the render method of If I have it right, I'll submit a PR for this fix later today. |
|
Something like this. But there's a few more similar places you'd need to change. |
|
Yep, that's what I understood. Will submit a PR soon. Cheers. |
* 'getDeclarationErrorAddendum' function of ReactDOMComponent.js * 'getSourceInfoErrorAddendum' function of ReactElementValidator.js * 'getDeclarationErrorAddendum' function of instantiateReactComponent.js and ReactElementValidator.js * 'traverseAllChildrenImpl' function of traverseAllChildren.js * 'attachRef' function of ReactRef.js * 'mountIndeterminateComponent' function of ReactFiberBeginWork.js * 'createFiberFromElementType' function of ReactFiber.js * 'getDeclarationErrorAddendum' function of ReactDOMSelect.js * 'unmountComponentAtNode' function of ReactMount.js * 'getDeclarationErrorAddendum' function of ReactControlledValuePropTypes.js * 'checkRenderMessage' function of CSSPropertyOperations.js * 'getDeclarationErrorAddendum' function of ReactDomFiberSelect.js * 'getCurrentComponentErrorInfo' function in 'ReactElementValidator' * 'getDeclarationErrorAddendum' function in ReactDOMFiberComponent.js Fixes facebook#8719
|
I caught all dynamically-generated warnings which contain the phrase "Check", but I also found similar warnings with different verbage so I'm doing a more fine-grained search now. If it's OK, I'll submit a follow-up PR if I find anything. |
|
Best to put them all in one PR I think. |
|
OK. So that I understand, the spirit of this bugfix is to only address the dynamically-generated warnings with large blocks of text, right? If a warning has dynamic content but is small, easily parseable, and clearly shows which file is problematic, there's no need for a newline, right? |
|
Yes. I want to fix those that are easy to miss and add info at the very end of a long message. |
|
@gaearon as requested, here's a ping to say that this PR is ready for review. Cheers! :) |
As proposed in #8495 (comment), I think we should find the warnings where we add a dynamic part to the end (like
"Check the render method of <...>") and delimit it with two newlines. This way it’s much easier to recognize in the middle of a bunch of errors:This is a good first issue to contribute. You would need to find
warning()calls that include additional info likeCheck the render method ofand add a couple of newlines. You'd also need to change the tests in case they fail.The text was updated successfully, but these errors were encountered: