Conversation
size-limit report 📦
|
905570f to
488ffe0
Compare
|
I requested a draft review because I need opinions on this stack but may not be around to merge this till next week. |
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
I thought a bit about the router provider and I like the idea of it! Initially, I was wondering if we could just get away with a url -> route map or cache. But the idea of having a "live" resolver for the framework SDKs that support resolving a url to a route is a good one! Bonus points that this is not something users have to configure but is (for now) baked into browserTracingIntegrations. We can see where exactly this makes more sense as we split up the integration into sub integrations.
I suggest running this by the framework WG since it will primarily concern framework SDKs. I looked into a couple of the stacked framework PRs and the changes look fairly minimal which is nice!
Just had some minor nits for now
| * Normalizes to a real `URL` so providers never have to parse, and relative locations (which memory | ||
| * routers hand around) resolve against the document. | ||
| */ | ||
| function toURLObject(url: string): URL | undefined { |
There was a problem hiding this comment.
l: I think we already have a helper like this in core 🤔
There was a problem hiding this comment.
Good catch, the narrower shape still fits this functionality
488ffe0 to
3754c50
Compare
That was one of my goals, breaking up the dependency between route parameterization and tracing because parameterization could be needed in logs/metrics/errors or whatever. This brings me to the next point, so it feels like we need a breaking change to break up this dependency in those frameworks, for example the Vue SDK accepts the router in The base API is fine, but I will need to do some more work on the stack in downstream SDKs.
Will post in the channel! |
|
This was a 9 PR stack. It's now only a few, I wanted to record the reasoning. Every provider ended up registered inside a tracing integration, which is the coupling the work was meant to break. The deletions I expected only unlock once
So these 2 PRs do the smallest useful thing, parameterization that doesn't need tracing and that only fits in Next.js and Remix because these are only ones that have decoupled implementations (by virtue of the routes being accessible globally), Astro may also fit as well. I think what I have here is a good base that we can either merge today and improve later, or expand on the base API to address these concerns. |
… name No router event fires on a bfcache restore, so the span falls back to the raw pathname. In a plain browser app that matches every other navigation, but in a framework app it is the only navigation span not named from a parameterized route. #23551 gives us `resolveCurrentRoute()` to fix it.
Framework SDKs can register a provider that resolves a URL to a low-cardinality route name, so integrations stop each reaching for the route their own way. Wires up `bfcacheIntegration` as the first consumer: its segment name ends up as a metric dimension, where an unparameterized URL is unbounded cardinality.
4f0b460 to
589ff5c
Compare
|
bugbot run |
Providers only read the pathname, search and hash, so they get those instead of a full `URL`, and core keeps a single URL parser.
…-utils` It is browser-only, and `browser-utils` is where web vitals will consume it. `@sentry/browser` re-exports it as the public API.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
It evicted the oldest entry whenever the map was full, even if the key was already in it, and left an updated key in its old position instead of making it the most recently used.
Its only consumer, `bfcacheMetricsIntegration`, is npm-only as well.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d758fc0. Configure here.
Framework SDKs know their provider at `init`, so passing it as an option has it in place before any integration sets up, without relying on an integration of its own. `setRouteProvider` still replaces it at runtime.
|
👋 @msonnb — Please review this PR when you get a chance! |
This branch has not been deployed
2 participants
Adds the route provider API to core so framework SDKs can register how to resolve a URL to a parameterized route, and wires up
bfcacheMetricsIntegrationas the first consumer since that is where the fragmentation currently produces a raw URL on a metric dimension.bfcacheMetricskeeps its pathname fallback, so nothing changes until a provider is registered.closes #23556