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

[LogBox] Add LogBox.isIgnoredLog() for expo remote logging integration #34476

Closed
wants to merge 2 commits into from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Aug 23, 2022

Summary

In Expo environment, we overwrite console log handlers and send logs to CLI terminal or Snack remote console. If users use LogBox.ignoreLogs() to suppress some logs, the log will suppress from LogBox but still send to CLI terminal. Toward this problem, the PR introduces a LogBox.isIgnoredLog() function for us to check whether we should ignore the log entry.

This PR will help us to unblock and fix the problem mentioned in #33557 (comment). (with example included)

Changelog

[General] [Added] - Add LogBox.isIgnoredLog() function to indicate whether a log is ignored

Test Plan

Unit Test

 PASS  Libraries/LogBox/__tests__/LogBox-test.js
  LogBox
    ✓ `isIgnoredLog` returns true for ignored log (4 ms)
    ✓ `isIgnoredLog` returns true for regexp based ignored log (3 ms)
    ✓ `isIgnoredLog` returns true for any messages when `ignoreAllLogs` was called (3 ms)
    ✓ `isIgnoredLog` returns false for non-matched ignored log (4 ms)
    ✓ `isIgnoredLog` returns false when throwing exception from log parser. (1 ms)

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels Aug 23, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Aug 23, 2022
@analysis-bot
Copy link

analysis-bot commented Aug 23, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,617,195 +22
android hermes armeabi-v7a 7,031,541 +26
android hermes x86 7,917,356 +23
android hermes x86_64 7,891,047 +19
android jsc arm64-v8a 9,494,880 +10
android jsc armeabi-v7a 8,272,595 +16
android jsc x86 9,432,770 +7
android jsc x86_64 10,025,823 +15

Base commit: 163171c
Branch: main

@Kudo Kudo marked this pull request as ready for review August 23, 2022 05:42
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 23, 2022
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cipolleschi
Copy link
Contributor

It seems that the /rebase command is not working properly... :/

@Kudo, could you try and rebase the PR? I should have fixed the CI issues. Thank you so much!

@Kudo Kudo force-pushed the expo-logbox-ignoredlogs branch from 3da7a23 to cee34bc Compare August 24, 2022 16:13
@Kudo
Copy link
Contributor Author

Kudo commented Aug 24, 2022

rebased. thanks for helping the review!

@analysis-bot
Copy link

analysis-bot commented Aug 24, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 163171c
Branch: main

@Kudo Kudo force-pushed the expo-logbox-ignoredlogs branch from cee34bc to afe66ee Compare August 25, 2022 00:33
@Kudo
Copy link
Contributor Author

Kudo commented Aug 25, 2022

there looks like a network issue from ci job. i'm rebasing again to test again.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Hm, I think that LogBox.ignoreLogs is supposed to also silence the logs to the console. Looking at the code, it appears that we do this for console.warn but only do it for console.error if it's from the warning module.

Instead of this new API, what if we update the code here to also check the ignored logs: https://github.com/facebook/react-native/blob/main/Libraries/LogBox/LogBox.js#L179

@rickhanlonii
Copy link
Member

This PR will help us to unblock and fix the problem mentioned in #33557 (comment). (with example included)

I also want to call out that the fix for this issue is not to ignore the warning. See the comment I left on the post.

@Kudo
Copy link
Contributor Author

Kudo commented Aug 26, 2022

Hm, I think that LogBox.ignoreLogs is supposed to also silence the logs to the console. Looking at the code, it appears that we do this for console.warn but only do it for console.error if it's from the warning module.

please let me describe the problem clearly.

  1. at first, LogBox.install() overwrites console.warn/console.error, so logs will pass to LogBox.
  2. then in expo environment, we overwrites console.warn/console.error again for remote logging, e.g. in snack.expo.dev, we pass logs to snack remote console. we will pass logs both to original log handler (LogBox) and remote console.

for the code snippet, LogBox.ignoreLogs(['foo']); console.warn('foo');. the message will be ignored by LogBox but show in expo remote console. so i'm thinking to propose LogBox.isIgnoredLog() for us to check whether the log is explicitly ignored by developers and should not pass to remote console.

at first glance, the LogBox has some internal ignored rules as you mentioned. for instance, message starts with (ADVICE) might be ignored and message doesn't start with Warning: will be ignored by console.error. i was thinking whether we should follow the complex logic as LogBox. what we need is to ignore the logs where developers explicitly ignore. in this case, if we think LogBox.ignoreLogs as LogBox.ignoreAdditionalLogs, then the current implementation of LogBox.isIgnoredLog is just something like LogBox.isIgnoredAdditionalLog.

hopefully this is clear to you and i am open to the implementation. let me know what's your thought and makes sense to you.

This PR will help us to unblock and fix the problem mentioned in #33557 (comment). (with example included)

I also want to call out that the fix for this issue is not to ignore the warning. See the comment I left on the post.

yep! i like your reply for deprecated-react-native-prop-types approach. the pr is not fixing the original issue. it's mainly addressing the expo remote console issue where LogBox.ignoreLogs(['foo']); console.warn('foo'); unexpectedly shown in the remote console.

@rickhanlonii
Copy link
Member

Instead of overwriting higher in the stack and passing to both, can you overwrite lower in the stack (i.e. before LogBox)? Then LogBox will be able to handle all of the interpolation and filtering before it gets passed to a lower level. That's how we handle the logs in Metro and it would probably be weird for users if they see a log in Metro but not in other tools.

@Kudo
Copy link
Contributor Author

Kudo commented Aug 26, 2022

from 8abe737068a5 of [email protected], LogBox.install happens in InitializeCore. that's a pretty early stage at metro getModulesRunBeforeMainModule. i am afraid something is not yet available at this stage. especially we have to access some native modules to get the remote logging endpoint to send logs.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@rickhanlonii
Copy link
Member

rickhanlonii commented Aug 30, 2022

@Kudo you have access to the bundler though right? You can inject stdio and lazily init the native modules?

@Kudo
Copy link
Contributor Author

Kudo commented Aug 31, 2022

@Kudo you have access to the bundler though right? You can inject stdio and lazily init the native modules?

@rickhanlonii that's achievable. it's just requiring additional setup. i may need to add an injection stdio script at metro's getModulesRunBeforeMainModule right before react-native InitializeCore. i was thinking if there're any solutions without touching metro. if there are no better solutions, i'm fine with it. do you have any idea in mind?

@rickhanlonii
Copy link
Member

I think that's probably the best thing to do here since you're instrumenting the low-level stdio. That means you won't have to re-implement all of the LogBox error parsing logic, which is pretty fragile and tied to specific React/React Native versions.

@brentvatne
Copy link
Collaborator

I caught up with @Kudo about this on a call, and it occurred to me that we may want to keep the current behavior in Expo projects - in part because of the complexity of the workaround (configuring Metro to inject code before InitializeCore) and also because it's not clear to me to me that it's always desirable to have LogBox.ignoreLogs prevent the default behavior of console.warn. It might be more predictable and flexible to treat these outputs distinctly - so you would filter LogBox with ignoreLogs and console by wrapping calls to console methods or patching them (as you would do in any other JS environment).

What do you think? I don't want to diverge from React Native on this point - would you be open to a PR to change this behavior in React Native?

@rickhanlonii
Copy link
Member

Hm, yeah good point. Let me ask @sshic, who has been working close to the error handling than I have.

@luluwu2032
Copy link
Contributor

luluwu2032 commented Sep 7, 2022

I think this is related to the purpose of LogBox, specifically is it for filtering all logs for all use cases or just for adding extra functionality for logging? I don't have a definite answer, nor I know what our thoughts were when LogBox was firstly introduced. In general I would support giving users/developers more flexibility, in this case letting LogBox.ignoreLogs filters only for LogBox data makes sense to me, but I slightly worry that it can be confusing for users/developers that calling LogBox.ignoreLogs()won't ignore logs in all places and they have to find other ways (for example checking with the new API LogBox.isIgnoredLog()) to manually achieve this. What do you think @rickhanlonii @brentvatne @Kudo ?

@rickhanlonii
Copy link
Member

Yeah great questions @sshic. When I implemented ignoreLogs it was originally just to move console.ignoreYellowBox over to LogBox. When I added the filtering for console messages too, my thought process was: Developers probably don't want to configure hiding logs twice, so the LogBox method can handle both.

Thinking more about this now, I think I made a mistake, and we actually should never hide console errors and warnings. The referenced issue #33557 is a great reason why - the correct fix for this is not to ignore the errors, it's to address the deprecation / removal of the API. Allowing an easy ignoreLog method to hide the problem made this migration much more painful.

So I think the right next steps would be:

  • Update LogBox to never filter console methods
  • Do not expose which logs are filtered in LogBox
  • Recommend that Expo does not add console filtering either

If folks want to override console methods to filter, can't stop them but it's not recommended.

What do you think @sshic @brentvatne @Kudo

@brentvatne
Copy link
Collaborator

@rickhanlonii - I agree with your reasoning and proposed next steps!

@cipolleschi
Copy link
Contributor

@rickhanlonii @Kudo Should we then close this PR?

@Kudo
Copy link
Contributor Author

Kudo commented Sep 15, 2022

thanks @cipolleschi! i would close this pr.

please try to follow up the mentioned next steps from Meta side when someone get a chance. thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Needs TypeScript Update p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants