Skip to content
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(next): add Astro.routePattern #11698

Merged
merged 8 commits into from
Aug 27, 2024
Merged

feat(next): add Astro.routePattern #11698

merged 8 commits into from
Aug 27, 2024

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Aug 14, 2024

Changes

For Astro 5.0
Closes PLT-2339

It adds a new field to the Astro global called routePattern.

route is RouteData.component without srcDir and pages:

  • src/pages/index.astro -> index
  • src/pages/blog/[post].astro -> blog/[post]

Testing

I added new tests

Docs

withastro/docs#9180

/cc @withastro/maintainers-docs for feedback!

Copy link

changeset-bot bot commented Aug 14, 2024

🦋 Changeset detected

Latest commit: f46dc07

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Aug 14, 2024
Copy link
Contributor

@github-actions github-actions bot left a 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.

@ascorbic
Copy link
Contributor

Definitely route. Component would be confusing.

@matthewp
Copy link
Contributor

Do we have use-case for this? I'm not against it, just wondering what it's for. I'm assuming it's something Starlight would want?

@ematipico
Copy link
Member Author

@matthewp If you check the linear ticket, Chris left a comment that explains the use case

@florian-lefebvre
Copy link
Member

As Chris said, I think it would more useful to have pattern than the component, so /blog/[...slug] instead of src/pages/blog/[...slug].astro

@bluwy
Copy link
Member

bluwy commented Aug 15, 2024

FWIW the recent astro:route:setup hook exposes a route object with a component property that would be like src/pages/blog/[...slug].astro. I used component because it's also what we (accidentally?) exposed on RouteData, but I don't know of a better name either.

@matthewp
Copy link
Contributor

component is completely legacy name from the very beginning of the project, I think route is fine here personally, it's niche and doubtful we'll regret it later.

@github-actions github-actions bot removed the pr: docs A PR that includes documentation for review label Aug 16, 2024
@ascorbic
Copy link
Contributor

ascorbic commented Aug 16, 2024

When I said I thought it should ne route, I was misunderstanding what it was for. If it is just the file, then why not Astro.file? I would expect route to be like @florian-lefebvre said: /blog/[...slug], not src/pages/blog/[...slug].astro

@ematipico ematipico force-pushed the feat/add-astro-route branch 2 times, most recently from 75550db to a30529a Compare August 16, 2024 13:48
@ematipico ematipico force-pushed the feat/add-astro-route branch from a30529a to 8df60cc Compare August 16, 2024 13:54
@bluwy
Copy link
Member

bluwy commented Aug 16, 2024

I actually quite like Astro.file (or Astro.route.file)? Also another data point, for injectRoute() it accepts a pattern option that would look like /blog/[...slug] as well.

@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Aug 16, 2024
@targetlucked69

This comment has been minimized.

@ematipico

This comment has been minimized.

@ematipico ematipico changed the title feat(next): add Astro.route feat(next): add Astro.routePattern Aug 20, 2024
@florian-lefebvre
Copy link
Member

  1. I'd like to know what the value is supposed to be in the following situations:
    a. inside a middleware (looks like it's an empty string?)
    b. after a rewrite (using next)
    c. after a rewrite (using ctx.rewrite)
  2. I'm a bit concerned about the name still. I think we may need to add more route related data in the future and it won't scale well (ctx.routePattern, ctx.routeComponent). So maybe we could have ctx.route.pattern instead?

@ematipico
Copy link
Member Author

  1. I'd like to know what the value is supposed to be in the following situations:
    a. inside a middleware (looks like it's an empty string?)

It will contain the value of the route you're about to render. I can add more tests if you want.

   b. after a rewrite (using next)

The updated value based on the payload provided to next

   c. after a rewrite (using ctx.rewrite)

The updated value based on the payload provided to rewrite

2. I'm a bit concerned about the name still. I think we may need to add more route related data in the future and it won't scale well (ctx.routePattern, ctx.routeComponent). So maybe we could have ctx.route.pattern instead?

Sounds good

@ematipico ematipico changed the title feat(next): add Astro.routePattern feat(next): add Astro.route.pattern Aug 21, 2024
@ematipico ematipico changed the title feat(next): add Astro.route.pattern feat(next): add Astro.routePattern Aug 21, 2024
@sarah11918
Copy link
Member

Just commenting that I will wait on the docs PR to the API reference page before reviewing the changeset here! 🙌

@florian-lefebvre
Copy link
Member

florian-lefebvre commented Aug 23, 2024

Should the route pattern have a leading slash? like to match RouteData route

@ematipico
Copy link
Member Author

@sarah11918 docs are up

@ematipico ematipico merged commit 05139ef into next Aug 27, 2024
13 of 14 checks passed
@ematipico ematipico deleted the feat/add-astro-route branch August 27, 2024 14:45
// src/pages/index.js

export const GET = (ctx) => {
console.log(ctx.routePattern) // it will log src/pages/index.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico I know this has already merged, but according to the line in docs about index.* showing only /, is this code comment about what's logged correct? It also includes the file extension here.

We will proofread all the changesets before 5.0 beta release anyway, so I'm not concerned about updating this right now. but I just want to make sure I'm understanding correctly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants