Skip to content

Expose YellowBox functionality to native code #4740

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.

Sign up for GitHub

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

Merged
3 commits merged into from
Apr 30, 2020
Merged

Expose YellowBox functionality to native code #4740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ffbcf5
Change files
asklar Apr 29, 2020
0bd2810
don't need sstream yet
asklar Apr 29, 2020
2c54989
const noexcept
asklar Apr 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions change/react-native-windows-2020-04-29-02-59-44-yellowbox.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "prerelease",
"comment": "Expose YellowBox functionality to native code",
"packageName": "react-native-windows",
"email": "asklar@microsoft.com",
"dependentChangeType": "patch",
"date": "2020-04-29T09:59:44.174Z"
}
8 changes: 5 additions & 3 deletions vnext/ReactUWP/Polyester/IconViewManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@ void IconShadowNode::updateProperties(const folly::dynamic &&props) {
if (propertyName == "color") {
if (IsValidColorValue(propertyValue))
glyphs.Fill(BrushFrom(propertyValue));
#if FUTURE
else if (propertyValue.isNull())
; // Log error, must have a color
#ifdef DEBUG
else if (propertyValue.isNull()) {
// Log error, must have a color
YellowBox("IconShadowNode - color property must be non-null");
Copy link
Contributor

@acoates-ms acoates-ms Apr 29, 2020

Choose a reason for hiding this comment

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

Generally Facebook only does this warning once per instance. You wouldn't want your app spammed with this error.
Unfortunately their warnOnce API isn't exposed to native. But then again, its probably not hard to do something on the native side to only do this once.

Copy link
Member Author

@asklar asklar Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@acoates-ms YellowBox already aggregates/coalesces like warnings into one and you just get one warning per message, with a badge for the # of times it has hit.
image

Copy link
Contributor

@acoates-ms acoates-ms Apr 29, 2020

Choose a reason for hiding this comment

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

I believe that it only does that if it matches the last error. So once we have multiple of these it would get very spammy.

Copy link
Member Author

@asklar asklar Apr 29, 2020

Choose a reason for hiding this comment

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

@acoates-ms confirmed that when I get the 53 test ones, then go to another page that shows a yellow box, then go to the 53 test yellowbox, the test yellowbox correctly adds those up and brings it to the top:

image

}
#endif
} else if (propertyName == "fontUri") {
if (propertyValue.isString()) {
Expand Down
7 changes: 7 additions & 0 deletions vnext/ReactUWP/Views/ShadowNodeBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,5 +154,12 @@ void ShadowNodeBase::EnsureHandledKeyboardEventHandler() {
}
}

void ShadowNodeBase::YellowBox(const std::string &message) const noexcept {
const auto instance = GetViewManager()->GetReactInstance().lock();
if (instance) {
instance->CallJsFunction("RCTLog", "logToConsole", folly::dynamic::array("warn", message));
}
}

} // namespace uwp
} // namespace react
2 changes: 2 additions & 0 deletions vnext/include/ReactUWP/Views/ShadowNodeBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ struct REACTWINDOWS_EXPORT ShadowNodeBase : public facebook::react::ShadowNode {
return false;
}

void YellowBox(const std::string &message) const noexcept;

ViewManagerBase *GetViewManager() const;
XamlView GetView() const {
return m_view;
Expand Down