-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
test(remix): Add Remix v2 future flags integration tests. #8397
Conversation
ed9de1b
to
1bf7942
Compare
size-limit report 📦
|
1bf7942
to
b166664
Compare
4e17b57
to
048426f
Compare
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.
Looking good! I like how you managed to keep the core application reusable.
I'm assuming the differences in output (mostly transaction names from what I can see) are because of Remix' weird new routing API?
@@ -12,7 +14,9 @@ test('should capture React component errors.', async ({ page }) => { | |||
expect(pageloadEnvelope.contexts?.trace.op).toBe('pageload'); | |||
expect(pageloadEnvelope.tags?.['routing.instrumentation']).toBe('remix-router'); | |||
expect(pageloadEnvelope.type).toBe('transaction'); | |||
expect(pageloadEnvelope.transaction).toBe('routes/error-boundary-capture/$id'); | |||
expect(pageloadEnvelope.transaction).toBe( | |||
useV2 ? 'routes/error-boundary-capture.$id' : 'routes/error-boundary-capture/$id', |
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.
I don't have a lot of context around React/Remix error boundaries so I'm wondering: Why is the transaction name different here? Shouldn't $id
always be a route segment? Or is this something that just currently doesn't work with our instrumentation and we need to adapt for v2?
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.
Looks like it's caused by the new route convention. I think we should address this in our transaction name logic for consistency. I'll check if we have access to any information about the final URLs inside our build-time route objects.
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.
Let's keep the dot notation for transaction names if they are using the new route convention.
I'd rather just match the schema that remix uses - this transaction name (with periods) also means it's much easier for users to go in and find the exact file that was generating this route.
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.
good points, Abhi. Then let's keep it.
@@ -8,5 +10,6 @@ test('should add `pageload` transaction on load.', async ({ page }) => { | |||
expect(envelope.contexts?.trace.op).toBe('pageload'); | |||
expect(envelope.tags?.['routing.instrumentation']).toBe('remix-router'); | |||
expect(envelope.type).toBe('transaction'); | |||
expect(envelope.transaction).toBe('routes/index'); | |||
|
|||
expect(envelope.transaction).toBe(useV2 ? 'root' : 'routes/index'); |
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.
Likewise, I'm assuming this is also something our current instrumentation just doesn't cover correctly in v2?
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.
<rant>eww, why these dot delimiters Remix 🥲 </rant>
No action required lol
Added an integration test application using Remix 1.17.0 and v2 future flags to see the state of current SDK support for v2.