Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(nuxt): Detect pageload by adding flag in Vue router #13171

Merged
merged 11 commits into from
Aug 5, 2024

Conversation

s1gr1d
Copy link
Member

@s1gr1d s1gr1d commented Aug 2, 2024

Nuxt is using the Vue router under the hood, but the previous condition to detect a page load (from.name == null && from.matched.length === 0) does not work with Nuxt, as from.matched is never empty.

@s1gr1d s1gr1d requested review from Lms24, mydea and a team August 2, 2024 10:06
Comment on lines +11 to +13
/* Make sure to import from '@nuxt/test-utils/playwright' in the tests
* Like this: import { expect, test } from '@nuxt/test-utils/playwright' */

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this just as a heads-up in case someone adds new tests later on.

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Sounds reasonable to me!

@@ -50,18 +50,27 @@ export function instrumentVueRouter(
},
startNavigationSpanFn: (context: StartSpanOptions) => void,
): void {
let IS_FIRST_PAGE_LOAD = true;
Copy link
Member

@Lms24 Lms24 Aug 2, 2024

Choose a reason for hiding this comment

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

l: Given that this is not a constant, I'd say we don't capitalize the variable name

Copy link
Contributor

github-actions bot commented Aug 2, 2024

size-limit report 📦

Path Size
@sentry/browser 22.46 KB (0%)
@sentry/browser (incl. Tracing) 34.24 KB (0%)
@sentry/browser (incl. Tracing, Replay) 70.29 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 63.63 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 74.69 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 87.29 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 89.13 KB (0%)
@sentry/browser (incl. metrics) 26.77 KB (0%)
@sentry/browser (incl. Feedback) 39.39 KB (0%)
@sentry/browser (incl. sendFeedback) 27.08 KB (0%)
@sentry/browser (incl. FeedbackAsync) 31.72 KB (0%)
@sentry/react 25.23 KB (0%)
@sentry/react (incl. Tracing) 37.24 KB (0%)
@sentry/vue 26.61 KB (0%)
@sentry/vue (incl. Tracing) 36.08 KB (+0.05% 🔺)
@sentry/svelte 22.59 KB (0%)
CDN Bundle 23.65 KB (0%)
CDN Bundle (incl. Tracing) 35.9 KB (0%)
CDN Bundle (incl. Tracing, Replay) 70.33 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) 75.59 KB (0%)
CDN Bundle - uncompressed 69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed 106.32 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed 218.17 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 231.08 KB (0%)
@sentry/nextjs (client) 37.09 KB (0%)
@sentry/sveltekit (client) 34.81 KB (0%)
@sentry/node 114.77 KB (0%)
@sentry/node - without tracing 89.33 KB (0%)
@sentry/aws-serverless 98.5 KB (0%)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

🚀

@@ -26,8 +28,8 @@ export function init(options: SentryNuxtOptions): Client | undefined {
// todo: the buildAssetDir could be changed in the nuxt config - change this to a more generic solution
if (event.transaction?.match(/^GET \/_nuxt\//)) {
options.debug &&
// eslint-disable-next-line no-console
console.log('[Sentry] NuxtLowQualityTransactionsFilter filtered transaction: ', event.transaction);
DEBUG_BUILD &&
Copy link
Member

Choose a reason for hiding this comment

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

🙏 good change!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants