refactored outbound links and file downloads #2109
Conversation
BundleMonUnchanged files (7)
No change in files bundle size Final result: ✅ View report in BundleMon website ➡️ |
There was a problem hiding this comment.
Looking good!
From my experience, a queue like inFlightRequests is more complex to manage than you think, especially in an async environment. I'd like the different scenarios to be verified and tested before we roll this out to all the customers. We should think of every way to mess up inFlightRequests and event.defaultPrevented behaviour and write a test for it.
…refactor-oubound-links-and-file-downloads
tracker/test/support/test-utils.js
Outdated
| @@ -14,6 +14,24 @@ exports.mockRequest = function (page, path) { | |||
| }) | |||
| } | |||
|
|
|||
| // Mocks a specified number of Plausible event requests. Returns a promise that resolves to a list | |||
| // of event names as soon as the specified number of requests is made, or 10 seconds has passed. | |||
| exports.mockManyPlausibleRequests = function(page, path, numberOfRequests) { | |||
There was a problem hiding this comment.
I like that you decided to make this a separate function. I think it makes more sense :)
tracker/test/support/test-utils.js
Outdated
| @@ -14,6 +14,24 @@ exports.mockRequest = function (page, path) { | |||
| }) | |||
| } | |||
|
|
|||
| // Mocks a specified number of Plausible event requests. Returns a promise that resolves to a list | |||
| // of event names as soon as the specified number of requests is made, or 10 seconds has passed. | |||
| exports.mockManyPlausibleRequests = function(page, path, numberOfRequests) { | |||
There was a problem hiding this comment.
As this is mocking Plausible requests only, we can probably omit the path argument, and replace it with "/api/event".
There was a problem hiding this comment.
Makes sense, removed the argument.
|
Happy with the code. It's a big change to a very sensitive and depended on piece of code. What can we do to verify that these changes:
Before and after pushing this live? |
Agreed, @ukutaht
I tested the new code via the page console on the site shared in the issue. My testing workflow was the following:
Tested environments:
The result was as expected - the outbound link event was sent, and unlike before, the lightbox opened nicely in a popup. I just read through the issue once again. The customer reported that the link opens in a new tab rather than the same tab which was actually the case in my testing. I've now asked for their confirmation about that here: #1941 (comment)
These changes currently only change outbound link and file download tracking and cannot break any other tracker functionality. The BrowserStack tests cover the core part of sending these custom events, but I guess it would also make sense to do some more manual testing on different environments with BrowserStack Live.
Not sure if there's any other way than waiting for user feedback? |
|
Closing this PR, as started from a clean slate in #2208 with smaller commits. |
I opened a new PR instead of continuing #1938, as I think it makes sense to start from scratch with the refactoring before moving on to the
tagged eventsextension.Changes
anyto represent anorstatement for multiple script extensionsinFlightRequestsqueue to avoid conflict when multiple Plausible events are sent at the same time (for example, a link can be both outbound and a file download)event.defaultPreventedis alreadytrue(this is the fix for outbound-links script causes media lightboxes to open in new tab #1941)Tests
Changelog
Documentation
Dark mode