Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Experimental Soft Navigation support #308
base: main
Are you sure you want to change the base?
Experimental Soft Navigation support #308
Changes from 14 commits
42ccabc
d995891
d2fc3ec
9b8202d
d2e0ab6
97fd350
03f8448
74b1231
50e9ded
dca3482
a387712
50d8dc1
5fa268c
4f558ee
01ab0ed
9e038c3
d12ce69
9f87d85
1700428
d0a4f19
c5984e5
9afc257
7e316eb
ecf82d4
f013531
2d7ea75
2e63c81
bb49653
60dde0b
aa45395
b4b28b0
f381326
e11c6b8
99d2b9b
5cdb8f9
090e7b2
39a043a
9c1e68c
5cc8b86
94dd1cc
f659c15
607041f
db653ae
173d87f
71c5927
9d87a20
b39d261
9835574
e373b67
5de5e89
7c00038
2dea9a2
1e72319
42b8e47
5b5c81c
1af7905
3ebf57f
5ab847d
8b8bd7f
44c6e07
cd5a2ee
f960498
57b751e
8a42838
a44dd93
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Perhaps a cleaner way to do it is to have the helper class that calls handleEntries automatically split entries based on navigationID?
Then, you dont' need to do this once per entry.
Also, that helper that does the split could call something like "finalizeAndReset", so we wouldn't need to do these tests here at all. The metric specific handlers would just get a URL and maintain a score...
WDYT?
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.
While that may look like cleaner code, I wonder about the performance implications of splitting up the array like that versus doing this check every time? In effect you’d need to do that check anyway for each entry AND copy the entries to a new array. Which sounds like a net negative to me.
WDYT?
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.
Followup from comment above... I think we should just automatically do this from inside the observe() helper, shared by all metrics
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.
Not all metrics need the soft-navigation observer (FCP and FID don’t for example as they can finalise immediately).
For those that do need this to finalise metrics that may not be finalised before then, there are different steps to make so it’s not generic across all the metrics. We could pass in a callback function to handle that, but that’s exactly what this currently does.
Also currently the observe function is for a single observer with a call back on what to do with that. I’m not sure it’s right to change that to also call another observer for some metrics, with a different callback function.
But maybe I’m just not seeing the way to do this? If you have a look at some of the other metrics and still think there’s an easier/cleaner way to do this, then definitely open to it, if you could give me a little nudge as to how you think it could be structure.