Skip to content

Fix logging and notifyOnLogging to work with Plotly.newPlot calls #4994

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

Closed
wants to merge 8 commits into from

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jul 9, 2020

Fixes #4555 thanks to #4555 (comment).

TODO:

  • test this from python
  • add a jasmine test

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Jul 9, 2020
@nicolaskruchten
Copy link
Contributor

If we're concerned about this refactoring shaking something loose at runtime that causes silent/subtle failures, could we try adding some extra runtime checks at the top of all the function definitions that we're changing which error out if the first argument is not as expected, and then run the test suite? That might flush out some extra silent failures? We wouldn't commit this code into master but as an extra one-time safety check?

Otherwise, would this be a safer change if we added an argument instead of changing the first one?

@archmoj
Copy link
Contributor Author

archmoj commented Jul 20, 2020

If we're concerned about this refactoring shaking something loose at runtime that causes silent/subtle failures, could we try adding some extra runtime checks at the top of all the function definitions that we're changing which error out if the first argument is not as expected, and then run the test suite? That might flush out some extra silent failures? We wouldn't commit this code into master but as an extra one-time safety check?

Otherwise, would this be a safer change if we added an argument instead of changing the first one?

@nicolaskruchten thanks very much for the feedback.

Adding this new gd parameter to the end of function arguments is not very helpful; while those functions may be called without full set of arguments.

On the other hand checking the first argument in the test looks like a good idea; although in the end we may not catch anything new since those tests are passing already. Anyway I could give it a try.

@archmoj archmoj requested a review from alexcjohnson July 20, 2020 14:36
@archmoj archmoj added the dash label Jul 30, 2020
@archmoj
Copy link
Contributor Author

archmoj commented Jul 30, 2020

Notes by @alexcjohnson:

I’m a bit uncomfortable with the logging PR - seems like it’s passing gd essentially everywhere, when there are really only a few places we call log/warn/error.

One thing we could do to make it easier is if you have access to any DOM node, allow the log functions to search its parents for a _context

And some of them, like randstr, we could probably just get rid of the log message, turn it into throwing an error.

@archmoj
Copy link
Contributor Author

archmoj commented Jul 30, 2020

Notes by @alexcjohnson:

And some of them, like randstr, we could probably just get rid of the log message, turn it into throwing an error.

Good call. Done in bddd218.

@archmoj
Copy link
Contributor Author

archmoj commented Jul 30, 2020

Notes by @alexcjohnson:

One thing we could do to make it easier is if you have access to any DOM node, allow the log functions to search its parents

Searching DOM elements for gd is little hacky (in my opinion) and could add to the complexity of this PR.

@archmoj archmoj closed this Nov 10, 2021
@archmoj archmoj deleted the fix4555-notify-on-logging branch November 10, 2021 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make notify-on-logging work with newPlot
2 participants