-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update current state in page load and navigation plugins #553
base: integration/component-rendering
Are you sure you want to change the base?
Update current state in page load and navigation plugins #553
Conversation
@@ -4,6 +4,7 @@ import type { BrowserConfiguration } from '../config' | |||
import type { RouteChangeSpanEndOptions, RouteChangeSpanOptions } from '../routing-provider' | |||
import { getPermittedAttributes } from '../send-page-attributes' | |||
import { defaultRouteResolver } from '../default-routing-provider' | |||
import type { AppState } from '../../../../core/lib/core' |
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.
This should be an import from @bugsnag/core-performance
and not a relative import
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.
So this import needs to be fixed in all plugins?
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.
yep, if we need to use code from another library, it should be exported and imported appropriately.
@@ -10,6 +10,7 @@ import type { BrowserConfiguration, BrowserSchema } from '../../lib/config' | |||
import { createDefaultRoutingProvider } from '../../lib/default-routing-provider' | |||
import type { OnSettle } from '../../lib/on-settle' | |||
import type { StartRouteChangeCallback } from '../../lib/routing-provider' | |||
import type { AppState } from '../../../../core/lib/core' |
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.
same as above, though not such a big deal in the tests
let appState: AppState = 'starting' | ||
const setAppState = jest.fn((state: AppState) => { | ||
appState = state | ||
}) |
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.
are you not able to get these from the plugins: (spanFactory) => []
parameter of the test client on line 57? they should be part of the base client so no need to mock them here
Browser bundle sizeNPM build
CDN build
Code coverage
Total:
Generated against 7f23a1c on 16 January 2025 at 13:09:15 UTC |
Goal
Automated component instrumentation
Changeset
Update page load plugins to set appState to
ready
after ending the app start span.Update navigation plugins to set appState to
navigating
when the route changes andready
when the navigation is complete.Testing
Update unit test to check if setAppState was called and changed