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
Lms24
left a comment
Lms24
left a comment
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?
| 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.
| 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.