test(remix): Add Remix v2 future flags integration tests. #8397
Conversation
onurtemizkan
force-pushed
the
onur/remix_future_flags
branch
from
ed9de1b
to
1bf7942
Compare
size-limit report 📦
|
onurtemizkan
force-pushed
the
onur/remix_future_flags
branch
from
1bf7942
to
b166664
Compare
onurtemizkan
changed the title
onurtemizkan
changed the title
onurtemizkan
changed the title
onurtemizkan
force-pushed
the
onur/remix_future_flags
branch
from
4e17b57
to
048426f
Compare
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
<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.