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

Add an option to DevTools to enable double-logging #19710

Closed
wants to merge 4 commits into from

Conversation

Jchinonso
Copy link

@Jchinonso Jchinonso commented Aug 27, 2020

Summary

We currently plan to suppress double-logging in development by overriding console in #18547. However, this can be confusing for some scenarios and double logging may be preferable in them. This change adds a toggle to DevTools that would allow to re-enable it on this screen:

90825360-b7372000-e330-11ea-9128-803d35d27e07

Test Plan

Screenshot from 2020-09-04 00-18-20

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4af8e58:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Aug 27, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 4af8e58

@sizebot
Copy link

sizebot commented Aug 27, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 4af8e58

@Jchinonso Jchinonso marked this pull request as draft August 27, 2020 18:58
@Jchinonso Jchinonso marked this pull request as ready for review August 27, 2020 19:18
@Jchinonso Jchinonso marked this pull request as draft August 27, 2020 19:18
@Jchinonso Jchinonso changed the title WIP Add an option to DevTools to enable double-logging Add an option to DevTools to enable double-logging Aug 28, 2020
@Jchinonso Jchinonso force-pushed the enabledoublelogging branch 3 times, most recently from d534a1a to 74e41ce Compare August 28, 2020 10:51
@bvaughn
Copy link
Contributor

bvaughn commented Aug 31, 2020

Hi @Jchinonso. What's the purpose of this change?

@Jchinonso
Copy link
Author

Hi @Jchinonso. What's the purpose of this change?

@bvaughn #19710 (comment)

@bvaughn
Copy link
Contributor

bvaughn commented Sep 1, 2020

There are two things that override the console:

  1. React itself, before double rendering (in strict mode, DEV only) (see Disable console.logs in the second render pass of DEV mode double render #18547)
  2. DevTools, before re-rendering a component to inspect its hooks (see Improving the component logging/debugging experience #15726)

Such a setting as the one you're adding here could be used to control the 2nd (DevTools) but I don't think anyone is asking for us to stop disabling the console in that case– so I assume you're targeting the first (React's disabling of the console, added in #18547).

That code wasn't written to be turned on or off externally (by something like DevTools). It's built into React. Allowing DevTools to control it (via a new setting like the one you're proposing) would require updating React as well.

Not sure how I feel about this feature 😄 but I'll be happy to look over the code when it's ready.

@Jchinonso
Copy link
Author

Jchinonso commented Sep 1, 2020

@bvaughn thanks for the clarification, I saw the patching on the console for both devtools and react and was confused on how to approach this. Now i know, i will take the React route

@Jchinonso Jchinonso marked this pull request as ready for review September 3, 2020 21:31
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I think we need to change a few things.

packages/shared/ConsolePatchingDev.js Outdated Show resolved Hide resolved
packages/shared/ConsolePatchingDev.js Outdated Show resolved Hide resolved
@Jchinonso Jchinonso force-pushed the enabledoublelogging branch 3 times, most recently from 3ec4571 to 86f58d3 Compare September 8, 2020 17:23
@Jchinonso Jchinonso force-pushed the enabledoublelogging branch 2 times, most recently from 6c5a6dd to 42dccfe Compare September 10, 2020 06:58
@stale
Copy link

stale bot commented Jan 9, 2022

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2022
@eps1lon
Copy link
Collaborator

eps1lon commented Feb 24, 2022

Replaced by #22030

@eps1lon eps1lon closed this Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants