-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[dev] Create toasts for console errors and warnings in dev mode. #192412
base: main
Are you sure you want to change the base?
Conversation
Pinging @elastic/appex-sharedux (Team:SharedUX) |
@spalger - That's one of my better screenshots, if I do say so myself. :elasticheart: |
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.
Is the plan to revert this after the React upgrade is completed? A method to disable this might be good if it is going to be around for awhile.
@Ikuni17 I actually want to keep it around. We have lots of warnings and errors that appear in the console-- that are only logged in dev mode-- and live for a very long time without being addressed. I'm actually curious why we'd disable it, given that we should be in the habit of fixing warnings and errors in our application? |
Regarding the React upgrade, my concern is a bit different. When we update, we’ll receive a warning for every ReactDOM.render call. This will be very noisy given the number of React trees we have. I don’t think spamming toasts for these warnings would be helpful, as they are expected during this transitional period. Instead, I wanted to dedupe these ReactDOM.render warnings in the console to minimize noise during the upgrade (print console warning only once) I also suggest to look at the toast notifications for console messages independently from the React upgrade. So suggest we have as follows:
|
@Dosant So the good news is:
I'll take this feedback into account, see if I can clean it up a bit in preparation for what you propose. I think simple |
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
thanks!
yes, sure, even str.startsWith() would be good enough for the particular message I have in mind.
I know, I wanted to suppress/dedupe a particular warning in console also |
I agree the warnings and errors should be fixed, and it is a good habit to get into. The toasts could even be opt out, but I don't think a dev should be required to use this. They may prefer to have the messages in browser dev tools only. Forcing the dev to see the toasts doesn't necessarily equate to issues being fixed, and some may find it a hinderance. Also, we likely want to disable this in FTR and Cypress tests to avoid test flake with elements not being visible or clickable. |
Summary
As titled. This addition will surface any
console.warn
orconsole.error
messages as toasts in Kibana in dev mode.This is essentially a pre-requisite for completion of #138222 by surfacing any issues to developers as they work or navigate.