-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[EBT] Add page url to browser-side context #159916
[EBT] Add page url to browser-side context #159916
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.
LGTM! Thank you for doing this!
I just added 1 question since I'm not familiar of how internal route-navigation is performed
await navigate('/app/app1/bar?hello=dolly'); | ||
await flushPromises(); | ||
await navigate('/app/app2#/foo'); | ||
await flushPromises(); | ||
|
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.
Q: would it work when navigating inside the same app?
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.
Yeah it does, but I can add a test to ensure that
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.
done in 06452c6
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.
self-review
const location$ = getLocationObservable(window.location, this.history); | ||
registerAnalyticsContextProvider({ | ||
analytics, | ||
location$, | ||
}); |
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.
The actual registration
const locationToUrl = (location: Location) => { | ||
return `${location.pathname}${location.hash}`; | ||
}; |
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.
Converting Location
to our actual url: using pathname
+ hash
history.listen((location) => { | ||
subject.next(locationToUrl(location)); | ||
}); | ||
return subject.pipe( | ||
startWith(locationToUrl(initialLocation)), | ||
distinctUntilChanged(), | ||
shareReplay(1) | ||
); |
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.
Emit with initial page's location + on each history change. Using distinctUntilChanged
to avoid emitting multiple time for the exact same url
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Part of #149249
Add a new EBT context providing the
page_url
field to events.page_url
is based on the current url'spathname
andhash
exclusively (no domain, port, query param...)