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

Remove React.error and React.warn #16126

Merged
merged 2 commits into from
Jul 12, 2019

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jul 12, 2019

Sebastian and I have just discussed my concerns around React.warn and React.error. The quick takeaway is this: I am going to remove React.warn and React.error for now. We can revisit these methods later as part of a larger discussion around the console logging experience with React (see #15726) but we should not block 16.9 for this.

In the meanwhile, I will be adding functionality to React DevTools to intercept calls to console.error, console.warn, and console.trace and auto-append component stacks (in DEV mode). This seems like a nice 80/20 solution that doesn't limit our options for a future API.

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

I love the proposed solution to auto-attach stacks. Solidddd.

@sizebot
Copy link

sizebot commented Jul 12, 2019

React: size: -0.2%, gzip: -0.4%

Details of bundled changes.

Comparing: 29b4559...adce079

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js -0.8% -0.7% 113.72 KB 112.76 KB 28.65 KB 28.43 KB UMD_DEV
react.production.min.js -0.2% -0.4% 13.25 KB 13.22 KB 5.09 KB 5.08 KB UMD_PROD
react.profiling.min.js -0.2% -0.4% 15.44 KB 15.41 KB 5.65 KB 5.63 KB UMD_PROFILING
react.development.js -1.3% -0.9% 71.22 KB 70.26 KB 18.59 KB 18.42 KB NODE_DEV
react.production.min.js -0.5% -0.6% 6.68 KB 6.65 KB 2.75 KB 2.74 KB NODE_PROD
React-dev.js -1.4% -0.9% 69.44 KB 68.48 KB 17.85 KB 17.69 KB FB_WWW_DEV
React-prod.js 🔺+1.9% 🔺+0.3% 16.84 KB 17.17 KB 4.43 KB 4.44 KB FB_WWW_PROD
React-profiling.js +1.9% +0.3% 16.84 KB 17.17 KB 4.43 KB 4.44 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 12, 2019

In the meanwhile, I will be adding functionality to React DevTools to intercept calls to console.error, console.warn, and console.trace and auto-append component stacks (in DEV mode).

While this does sound like a good solution for experienced developers it still requires additional setup and navigation for novice developers. Debugging react shouldn't require a separate extension for the most basic things (like a "call"stack).

Is this really just removed because some rogue libraries might parse the component stack? Should we really optimize against possible hacks instead of for "normal" users?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 12, 2019

While this does sound like a good solution for experienced developers it still requires additional setup and navigation for novice developers. Debugging react shouldn't require a separate extension for the most basic things (like a "call"stack).

React DevTools are not just for "experienced developers" but all levels 😄 The component stack, while very helpful, is not required for debugging React apps. (It hasn't existed up until this point, after all.)

Is this really just removed because some rogue libraries might parse the component stack? Should we really optimize against possible hacks instead of for "normal" users?

No. I linked to #16017 which specifies a few concerns. Probably my biggest at this moment is that overriding only these two console methods also feels like an incomplete solution to the larger logging story (#15726).

@bvaughn bvaughn merged commit 8d413bf into facebook:master Jul 12, 2019
@bvaughn bvaughn deleted the remove-getComponentStack branch July 12, 2019 22:41
@eps1lon
Copy link
Collaborator

eps1lon commented Jul 12, 2019

React DevTools are not just for "experienced developers" but all levels

I was concerned with the existing setup. If I start out with react it's highly unlikely I have the devtools set up.

The component stack, while very helpful, is not required for debugging React apps. (It hasn't existed up until this point, after all.)

I'm not sure about the specific timeline but it existed for a long time in react itself (react specific warnings or prop-types).

I fear this change is too tunnel visioned on react-devtools only. It doesn't ship in the browser by default.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 13, 2019

If I start out with react it's highly unlikely I have the devtools set up.

In this scenario, the first time you ran React in development mode you would see this message in the console:

Screen Shot 2019-07-12 at 7 09 06 PM

I'm not sure about the specific timeline but it existed for a long time in react itself (react specific warnings or prop-types).

The thing that has not existed (because we never actually released these APIs) is the ability to log custom errors or warnings in user land with component stacks attached.

There's a balance to be struct between adding more things to React (which adds weight to the framework, even for the majority of people who use React apps and are not developers) and adding things to the surrounding tooling (which is opt-in and only impacts developers).

We don't make decisions like this lightly. There were several rounds of conversation between various parts of the team, and a couple of proposals (written by me). There is also still an outstanding issue (linked to in this PR) to come up with a more comprehensive logging plan.

This decision feels like the right set of trade offs at this moment in time though (in preparation for the 16.9 release).

@babangsund
Copy link

@bvaughn Was this originally inspired by the warning and invariant? 😄

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 13, 2019

@babangsund Not directly, not really. More so by console.warn and console.error TBH.

fabriziocucci added a commit to fabriziocucci/react-native that referenced this pull request Jan 19, 2024
Summary:
T46547044 has been closed and apparently `React.warn` was removed in [facebook#16126](facebook/react#16126).

Changelog: [Internal]

Reviewed By: cortinico

Differential Revision: D52907508
fabriziocucci added a commit to fabriziocucci/react-native that referenced this pull request Jan 19, 2024
Summary:

T46547044 has been closed and apparently `React.warn` was removed in [facebook#16126](facebook/react#16126).

Changelog: [Internal]

Reviewed By: cortinico

Differential Revision: D52907508
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jan 19, 2024
Summary:
Pull Request resolved: #42395

T46547044 has been closed and apparently `React.warn` was removed in [#16126](facebook/react#16126).

Changelog: [Internal]

Reviewed By: cortinico

Differential Revision: D52907508

fbshipit-source-id: a3621d876f904339791ab184a904e81f7a50b988
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants