-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(core): add routePattern
to GetStaticPathsOptions
#13520
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
feat(core): add routePattern
to GetStaticPathsOptions
#13520
Conversation
🦋 Changeset detectedLatest commit: 7777fb1 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
CodSpeed Performance ReportMerging #13520 will not alter performanceComparing Summary
Footnotes |
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.
Blocking because this isn't a patch, but a feature
routePattern
to GetStaticPathsOptions
Thank you @openscript for the PR. It's not very clear what this new feature is for. I read the description, but unfortunately, it doesn't provide a good enough example of how users should consume the new parameter. As for the tests, there's a contribution guide that should help. As for where, you could create a new file or extend the existing one. You will have to create new fixtures or files where you use the new feature, and execute certain assertions based on what these new files do. Tests are beneficial to us maintainers too, so we see how the new features are used. Also, you will be in charge of creating docs, if the PR is accepted. |
Thank you @ematipico for guiding me. Let's proceed like this:
I've already updated the PR description and I kindly ask you to reread it. With this PR a helper function for writing |
This would be helpful! How can I help to bring this along? |
For this specific case, I wonder if we could instead make sure |
Isn't it possible to do something like this? const routePattern = Astro.routePattern;
export function getStaticPaths() {
// do something with routePattern
return []
} |
No you can't reference stuff from the frontmatter outside of |
Ah, yeah, I misremembered how I used the collections inside the function. |
Related function:
Happens too early, that would require a compiler change + currying: -staticPaths = await mod.getStaticPaths({ paginate })
+staticPaths = await mod.getStaticPaths({ ...astroStaticPartial, routePattern: route.route })({ paginate }) -const $$Astro = $$createAstro();
-const Astro = $$Astro;
-export async function getStaticPaths({ paginate }) {
+export const getStaticPaths = (Astro) => async function getStaticPaths({ paginate }) {
const posts = await getCollection('blog');
console.log(Astro)
return posts.map((post) => ({
params: { slug: post.id },
props: post,
}));
} |
if we deprecate |
Hey again! Sorry it took so much time to get to this, but the core team agreed to deprecate
|
a4dc5f5
to
fad5d46
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.
This PR is blocked because it contains a minor
changeset. A reviewer will merge this at the next release if approved.
As for docs content, yes, I think another section after paginate sounds like a good idea on that page! Looking at the existing routing reference page and seeing that it's not all exactly formated as we've now started to form our reference pages, so I think we can be a little flexible about what we put on this page until we (probably @ArmandPhilippot !) decide how to standardize it so that it doesn't have to hold up this feature documentation. 😅 I will note that we have existing docs for what I think it might be helpful if this section can describe a specific case of accessing and using the route data inside
That's useful docs content to have either way, even if we eventually move content like that to the Routing Guide page (instead of reference) and make this a more traditional API reference entry. So, I think getting that written now is helpful, while we want to show people how to use the new feature, in addition to just providing a strict reference entry. Armand, what are your thoughts on this? |
I agree with your suggestion and moving "Data passing with props" and the new "Accessing route data" section to Routing! This looks more like guidance than reference to me. But this doesn't have to be done with this PR, so updating https://docs.astro.build/en/reference/routing-reference/#getstaticpaths as Florian suggested sounds like the right place! Re:
Yeah, this is a bit tricky here because the undocumented types rely on Typescript generics (and user's project). I thought I've added a comment regarding |
Alright then @openscript feel free to ping me once you open that docs PR! Doesn't have to be perfect, even something minimal is perfectly fine to get things started |
I tried my best to document the Let me know what you think about it. I'm happy to tailor it further. |
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.
Just noting that I'm working on the docs, then will be in here for beefy, exciting, changeset goodness! 🙌
Could you add a test for this? I think what you can do is:
|
Thank you for all the feedback! I'll work on the documentation and also add some tests in the next few days. |
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.
Approving code wise! Thanks a lot
Noting that we're getting very close to finalizing the docs on this one, which means we will be close to knowing what we want to put in the changeset! 🥳 |
Hello @florian-lefebvre @sarah11918 @ArmandPhilippot I just wanted to express that I enjoyed to work the last few days on this issue. Especially participating in writing the the documentation and reflecting on how to express things in a way the reader catches the idea, was fun. Hopefully we have the chance to work together on another task someday soon. Cheers 🥂 |
Glad you enjoyed! If you're interested, we're working towards Astro 6 and there are tasks to pick: #14383 |
This issue was now added to the |
Yes, good catch! This is indeed for |
I've updated the docs PR. 😄 |
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.
Approving for docs! 🚀
Changes
getStaticPaths()
is run in another scope, that makes it impossible to access Astro render context. For some use cases of implementinggetStaticPaths()
, it becomes necessary to know theroutePattern
to calculate the params and props for each page route.For example imagine you have a route
[...locale]/[files]/[slug].astro
and you want to implement agetStaticPaths()
that,[...locale]
[files]
according to the locale, so you can have a path translation[slug]
To lookup what
[files]
should be substituted with, you need to parse theroutePattern
as well as for calculating the translations.This change provides
routePattern
viaGetStaticPathsOptions
. It isn't a breaking change as whoever wants to consume it can, but doesn't need to.A workaround would be calculating this props during rendering of the actual page. For each render
getCollection()
needs to be invoked again. Then I would also wonder whyprops
are returned fromgetStaticPaths()
after all.Example code
[...locale]/[files]/[slug].astro
site.config.ts
translation-path.ts
Testing
I don't know how to test this. I'm very happy to add tests, if you can point me in the right direction.
Docs
withastro/docs#12316
/cc @withastro/maintainers-docs for feedback!
The new options should be mentioned in https://docs.astro.build/en/reference/routing-reference/#getstaticpaths.