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

fix: capture PerformanceNavigationTiming events even when window.load fires before plugin loads #81

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

adebayor123
Copy link
Member

Modified NavigationPlugin to capture PerformanceNavigationEvents when the plugin loads after the page does.

  • Removed PerformanceObserver from eventListener.
  • Added two new unit test cases and modified existing one case with respect to the new implementation change.
  • Created a new app called delayed_page.html that loads the script after the page finishes loading.
  • Created a new integ test case using the delayed_page app to verify that PerformanceNavigationEvents are captured.

To test the change, do the following:

  • Create a temporary app (delayed_page.html) in your local repository without any other implementation changes
  • Run server and either use Chrome debugger or open up local host, compare the output between index.html and delayed_page.html.
  • index.html should have the navigation events, while the delayed_page.html does not.
  • This indicates that with the original implementation, delayed_page properly simulates a web page loading prior to plugin
  • Once this is verified, pull in the NavigationPlugin changes, and rerun the server/debugger to see that now PerformanceNavigationEvents are captured in the delayed_page app.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines +66 to +69
if (
window.performance &&
window.performance.getEntriesByType(NAVIGATION).length
) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this guarantee that window.load has fired?

Or should we look for a specific event that signals the page has finished loading -- i.e., PerformanceTiming.loadEventEnd or PerformanceNavigationTiming.loadEventEnd

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline; The following line to the if statement uses PerformanceNavigationTiming.loadEventEnd

entryTypes: [NAVIGATION]
});
this.performanceNavigationEventHandlerTimingLevel2(
performance.getEntriesByType(NAVIGATION)[0]
Copy link
Member

Choose a reason for hiding this comment

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

Is it guaranteed that there is only one navigation timing entry?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I will change it to iterate instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, just found this: https://www.w3.org/TR/navigation-timing-2/
"Only the current document resource is included in the performance timeline; there is only one PerformanceNavigationTiming object in the performance timeline."

@qhanam
Copy link
Member

qhanam commented Feb 3, 2022

Please modify the commit message to use the following conventions:

  1. Use conventional commit format
    i.e.,<type>[optional scope]: <description>
  2. Use the imperative mood in the subject line
    i.e., a commit message subject line should be able to complete the sentence "If applied, this commit will commit message subject line"

@adebayor123 adebayor123 changed the title Fix to allow plugin to correctly capture performance navigation event… fix: capture PerformanceNavigationTiming events even when window.load fires before plugin loads Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants