-
Notifications
You must be signed in to change notification settings - Fork 806
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
chore: remove unload event from OTLPExporterBrowserBase #4438
chore: remove unload event from OTLPExporterBrowserBase #4438
Conversation
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.
Hi there, thanks for opening the PR. 🙂
Looks like a changelog entry is missing, you'll need to add an entry to ./CHANGELOG.md
to contiune.
experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts
Show resolved
Hide resolved
I don't think we should just be removing the "unload" event. I understanding the ask but there is a bunch of nuances that are bing missed here.
Rather than "remove", this "should" be configuration driven and we should include the others. Happy to consider that the "default" is And while it's being "deprecated" / "removed" the code should "handle" (identify) when the event was not able to be hooked (because the addEventListener returns false). Simply, just Removing this should also be considered a breaking change because of the downstream functionality change. |
I guess I'm confused, what was being proposed in your comment here? #4326 (comment) |
This PR just deleting the code is not how this should be working. It should be behind a flag about whether to add / remove the |
Okay thanks for the further explanation, do you mind describing or pointing to an existing example of how |
I'm having a very difficult time trying to figure out what you're asking for ... can you tell me more about the addEventListener returning false? In the MDN docs it says it just returns
I've updated the original PR to take an array as an optional argument -- otherwise it should perform just as it did before. |
@eldavojohn we discussed this in the SIG today and concluded that we should be fine just removing the functionality, but we want to list it as a breaking change. The possible break is this:
Please let me know if you have questions. Can probably close #4326 |
Hey @dyladan thanks ... um, I keep reading "removing the functionality" and I'm just absolutely confused. In the PR I closed, I allowed the event to be defined by the consuming code. That seemed to be wrong. So in this PR I remove the functionality. Which also seems to be wrong. I understand the breaking change from "removing the functionality" but I am absolutely struggling with what that means if it doesn't mean deleting the event listener that is listening for the |
I think you mixed up the PRs? This one doesn't allow any property to be set. This PR is the correct solution based on what we discussed in the meeting today. |
Sorry I definitely did, I thought this PR was horrendously wrong as explained by @MSNev . Please let me know if there's anything else I can do or any changes to this code. Thanks again! |
We discussed this with @MSNev today and he is now convinced this is the correct approach |
…nder the scenario in comments
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4438 +/- ##
==========================================
+ Coverage 92.41% 92.43% +0.01%
==========================================
Files 330 330
Lines 9525 9525
Branches 2033 2033
==========================================
+ Hits 8803 8804 +1
+ Misses 722 721 -1 |
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
…ry#4438) * Add the ability for the implementation to provide the binding event for shutdown. * fix: whoops totally missed the linting formatting fix step * Remove any concept of a shutdown event * add change log and obey interface * no longer need globalthis * modify the correct changelog and identify this as a breaking change under the scenario in comments * markdown lint * Update experimental/CHANGELOG.md Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> --------- Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
As discussed in #4326 we could remove any concept of a shutdown event and allow consuming code to handle that explicitly.
Which problem is this PR solving?
The 'unload' event is stopping any site using OTEL in JavaScript from taking advantage of back/forward cache. And also Google is planning to deprecate unload.
Fixes # (issue)
Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: