-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Added groupCollapsed polyfill #21457
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
eslint
found some issues.
Libraries/polyfills/console.js
Outdated
@@ -520,6 +520,12 @@ function consoleGroupPolyfill(label) { | |||
groupStack.push(GROUP_PAD); | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prettier/prettier: Delete ⏎
@@ -537,6 +543,7 @@ if (global.nativeLoggingHook) { | |||
table: consoleTablePolyfill, | |||
group: consoleGroupPolyfill, | |||
groupEnd: consoleGroupEndPolyfill, | |||
groupCollapsed: consoleGroupCollapsedPolyfill, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be the consoleGroupPolyfill
. I believe console.groupCollapsed
should behave the same as console.group
in a non-debugger environment logging to the OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @newyankeecodeshop if I use consoleGroupPolyfill
then the log won't be collapsed because of GROUP_OPEN
inside consoleGroupPolyfill
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eyesonly88 I don't understand how the GROUP_OPEN
character vs. GROUP_CLOSE
makes a difference about whether the group is shown collapsed. When you attach the Safari debugger to the JSContext, is it still using the console polyfill?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@newyankeecodeshop Yes it is using the console polyfill. Without my code, there is no groupCollapsed
. As you can see in the demo, using GROUP_CLOSE
makes it collapsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
groupCollapsed
isn't there because the console object in the JSC is not the original console object. The original one is called after using a polyfill method;
react-native/Libraries/polyfills/console.js
Lines 541 to 549 in 3592122
Object.keys(console).forEach(methodName => { | |
const reactNativeMethod = console[methodName]; | |
if (originalConsole[methodName]) { | |
console[methodName] = function() { | |
originalConsole[methodName](...arguments); | |
reactNativeMethod.apply(console, arguments); | |
}; | |
} | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Thanks for the PR. Seems like it previously fell through the radar. Sorry about that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@Eyesonly88 merged commit 638d672 into |
Summary: `groupCollapsed` is used to group logs but collapsed which is very useful when the console has many logs (e.g. when using `redux-logger`). For developers who prefer to debug iOS apps using Safari JSC, the `groupCollapsed` was not polyfilled. Fixes facebook#21446 Pull Request resolved: facebook#21457 Differential Revision: D13761796 Pulled By: cpojer fbshipit-source-id: e7c4f1ff4728c6a7f6ffd6cc629f7fbc1aa67e87
groupCollapsed
is used to group logs but collapsed which is very useful when the console has many logs (e.g. when usingredux-logger
).For developers who prefer to debug iOS apps using Safari JSC, the
groupCollapsed
was not polyfilled.Fixes #21446
Test Plan:
I tested it in the Safari Developer console.
Input:
Output:
A single collapsed log containing the other logs as expected. See demo below.
Demo:
Release Notes:
[GENERAL] [ENHANCEMENT] [JSC] - Added
groupCollapsed
to the console polyfill to enable log collapsing in consoles such as Safari