-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nuxt): Parametrize SSR routes #16843
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
Conversation
ea09c04
to
8492c82
Compare
I still need to look into why this is failing in |
} | ||
|
||
const method = ssrContext?.event?._method || 'GET'; | ||
const parametrizedTransactionName = `${method.toUpperCase()} ${routeInfo.parametrizedRoute}`; |
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.
m: We create the parametrizedTransactionName
but don't update the span name?
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.
oh that's left from before the last commit 😅
was using updateSpanName
before, but I only need to set 'http.route': routeInfo.parametrizedRoute
to update the name.
* An array of NuxtPage objects representing the build-time pages data. | ||
* Example: [{ name: 'some-path', path: '/some/path' }, { name: 'user-userId', path: '/user/:userId()' }] | ||
*/ | ||
export function extractParametrizedRouteFromContext( |
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.
l: should we memoize this function?
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.
Yeah the function is quite expensive as the lookup is hard :/
I added some general, small optimizations for the function.
Using the currentUrl
is not possible as it includes the parameters and that would blow up.
And the only way to memoize the result is using the ssrContextModules
as key (which is quite complex as this can be a large Set) and this Set has a different reference, every time a request is sent. So the key has to be created on every request, as I cannot use the Set itself as a key.
However, as I think the key generation regardless its overhead still makes sense, I added it now.
packages/nuxt/src/module.ts
Outdated
}); | ||
|
||
nuxt.hooks.hook('pages:extend', pages => { | ||
pagesDataTemplate.getContents = () => `export default ${JSON.stringify(pages, null, 2)};`; |
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.
m: So stringifying all of the pages can get pretty big from what I can see in the NuxtPage
type.
Perhaps we should just save a subset? We really only need the file
and the path
from looking at extractParametrizedRouteFromContext
.
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 catch, I also filtered for only dynamic pages.
# Conflicts: # dev-packages/e2e-tests/test-applications/nuxt-3-dynamic-import/tests/tracing.test.ts # dev-packages/e2e-tests/test-applications/nuxt-3-min/tests/tracing.test.ts # dev-packages/e2e-tests/test-applications/nuxt-3-top-level-import/tests/tracing.test.ts # dev-packages/e2e-tests/test-applications/nuxt-3/tests/tracing.test.ts # dev-packages/e2e-tests/test-applications/nuxt-4/tests/tracing.test.ts
9aff99a
to
b3bbf0c
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.
Great work! Had some questions about our matching algorithm. IIUC we need a similar logic in Next and possibly other frameworks at some point. Might make sense to think about extracing this in the future because matching routes is far from trivial. But obviously this is a something to follow up on later :)
file: '/app/pages/test-param/[param].vue', | ||
}), | ||
], | ||
expected: { parametrizedRoute: '/test-param/:param()' }, |
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.
l/q: what's the reason for the brackets in the parameters (:param_()_
)? Is this a Nuxt routing covention/something users are used to?
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.
This is how Nuxt lists parametrized routes - I would leave it like it is to not stumble into edge cases when modifying the route. But the convention is to use brackets [param]
in the filename.
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 think it's fine since we don't have a standard around how we format parameter path segments. I just haven't seen this in other frameworks before. thx for explaining
}); | ||
|
||
describe('multiple routes matching', () => { | ||
it('should find the correct route when multiple routes exist', () => { |
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.
sticking with this example: If I have two routes,
file: '/app/pages/test-param/user/[userId].vue',
file: '/app/pages/test-param/user/abc.vue',
do we still correctly identify the respective routes for urls /test-param/user/123
and /test-param/user/abc
?
(Sorry in case I missed such an example in already existing tests. If not, it might be a good test case)
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.
Related but not also rather a question: Does this handle catch-all routes?
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.
Both of that should work - I would add those two cases in another PR as I don't have tests for that yet and that would blow up the PR :D
Test follow-up for this PR: #16843 Re-organizes the unit tests a bit to be less repetitive with default data that is aligned to real-world examples. --- Adds unit tests for cases mentioned [here](#16843 (comment)): - differentiate dynamic vs static route on the same path (`users/:id` vs `users/settings`) - cases for catch-all routes
Adds route parametrization to SSR server routes.
The parametrized route data is gathered during build time and saved in a virtual file (added with
addTemplate
) which can hand-over the data to be accessed during runtime.The
nuxt-3-min
test (Nuxt 3.7) shows that the route parametrization does not work yet with this version. From Nuxt 3.9 onwards, it works. This is fine, as most people are on a more recent version anyways.part of #16684