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

Sitemap: support SSR routes #6534

Merged
merged 6 commits into from
May 3, 2023
Merged

Conversation

atilafassina
Copy link
Contributor

@atilafassina atilafassina commented Mar 13, 2023

Changes

  • Adds support for SSR routes
  • Changeset added

Testing

I tested using the examples, replacing in package.json the @astrojs/sitemap version with workspace:* and making sure it's using output: "server" in the astro.config.mjs.

Optional: adding const prerender = true to a few routes.

Run astro build and make sure a sitemap.xml is created with no duplicated routes and every SSR route is created.

Optional-2: Add a hardcoded customPages definition to sitemap options.

Docs

If you wish to use this implementation, I'm happy to update the docs.

/cc @withastro/maintainers-docs for feedback!

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Mar 13, 2023
@changeset-bot
Copy link

changeset-bot bot commented Mar 13, 2023

🦋 Changeset detected

Latest commit: 4e00db6

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

@atilafassina atilafassina force-pushed the sitemap-ssr branch 2 times, most recently from 1b73cea to ec7aac0 Compare March 13, 2023 15:17
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

I don't think we need a build:start hook with custom logic for this! build:done probably has most of what you need already, and if there's anything missing we can work on exposing it.

packages/integrations/sitemap/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/sitemap/src/index.ts Outdated Show resolved Hide resolved
@atilafassina atilafassina force-pushed the sitemap-ssr branch 6 times, most recently from 6b77de2 to 9d2af5c Compare March 13, 2023 19:23
@atilafassina
Copy link
Contributor Author

I don't think we need a build:start hook with custom logic for this! build:done probably has most of what you need already, and if there's anything missing we can work on exposing it.

Thanks for the insights, @natemoo-re . I refactored accordingly.
I must admit I still can't find a way to run getStaticPaths or to output the paths for the SSR'ed dynamic routes. Did I missed something perhaps?

@delucis
Copy link
Member

delucis commented Mar 14, 2023

getStaticPaths is not used for SSR sites so that’s maybe the issue? The model is quite different for dynamic routes in SSR. Instead of declaring a static array of supported routes via getStaticPaths, a dynamic route is expected to handle any matching parameter, e.g. src/pages/[lang]/index.astro will be used to serve any request that matches whether it is /en, /zh or /foo. It is then up to the developer to check Astro.params.lang and decide how that changes what is returned (i.e. whether /foo should be a 404 etc.)

I’d guess this makes it impossible for us to support these dynamic SSR routes in the sitemap.

@atilafassina
Copy link
Contributor Author

atilafassina commented Mar 14, 2023

getStaticPaths is not used for SSR sites so that’s maybe the issue? The model is quite different for dynamic routes in SSR. Instead of declaring a static array of supported routes via getStaticPaths, a dynamic route is expected to handle any matching parameter, e.g. src/pages/[lang]/index.astro will be used to serve any request that matches whether it is /en, /zh or /foo. It is then up to the developer to check Astro.params.lang and decide how that changes what is returned (i.e. whether /foo should be a 404 etc.)

I’d guess this makes it impossible for us to support these dynamic SSR routes in the sitemap.

Oh 🤯. Thanks a lot for this clarification @delucis
I was thinking on determining preemptively which routes should throw 404 and which should render.
But if it happens at render time, you're absolutely right.

The ideal scenario would be (1) for the developer to use customPages if they want to include such routes in the sitemap, or (2) create a additional entry to the sitemap where they append an entry to the xml when the page is generated. But case n.2 should not be covered by this integration, imho.


In this case I'll just get CI to pass 😊

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

This is looking really good! Happy to help debug CI failures if you're stuck, just let me know.

@atilafassina
Copy link
Contributor Author

This is looking really good! Happy to help debug CI failures if you're stuck, just let me know.

Thanks, @natemoo-re

I'm afraid I'll need your input on this one.
I fell into sort of a rabbit hole and here's what I found out:

route.pathname and route.route don't take addTrailingSlash into consideration. So I started using route.generate(route.pathname) which seemed to be the right way.

But the generator does not take into consideration buildFormat. So when it's "directory" it should add the trailing slash even when addTrailingSlash: ignore.

At this point I wanted to stop and ask you: is it desirable to patch the getRouteGenerator. But before asking I decided to try it out, the solution which seems to work for me is:

	/**
	 * If `addTrailingSlash` is 'always'
	 * or `buildFormat` is 'directory' and `addTrailingSlash` is not "never"
	 */
	const shouldAddTrailingSlash =
		addTrailingSlash === 'always' ||
                (buildFormat === 'directory' && addTrailingSlash !== 'never');
	let trailing: '/' | '' = '';
	if (shouldAddTrailingSlash && segments.length) {
		trailing = '/';
	}

but this breaks 1 test: astro-root-pagination-spread.

Additionally, deserializeRouteData doesn't have build format available there, so my patch into the generator had to add "directory" as a default value for testing my theory, so I'm not sure if that's a reliable solution.


So, what do you think I should do?
Of course, alternatively I can simply add my check on the integration level and tests will pass.

I just would like to have context if routeData not using build format for generating the path is intentional.

@atilafassina atilafassina force-pushed the sitemap-ssr branch 3 times, most recently from 54ee3d3 to aa4efae Compare March 16, 2023 14:17
@atilafassina atilafassina requested a review from natemoo-re March 22, 2023 07:29
@atilafassina atilafassina force-pushed the sitemap-ssr branch 2 times, most recently from 8a5a828 to 33dd8ca Compare March 28, 2023 21:14
@matthewp
Copy link
Contributor

@atilafassina sorry for not getting back to you. routeData is not "normalized" to take into account trailing slash config, you are right on that. We do that at the "end" of things, such as when static files are generated. So I think you are right to do it in the integration itself.

Let me know when this is ready for another review.

@atilafassina atilafassina force-pushed the sitemap-ssr branch 5 times, most recently from f625c1d to 45a8d5c Compare April 16, 2023 13:24
@atilafassina
Copy link
Contributor Author

pinging both @ElianCodes and @matthewp since both of y'all showed interest in it.
Thanks a lot, folks. Tests are now 🟢.

I had some weird situation with corrupted pnpm-lock.yaml from one of the rebases I did and that was causing unrelated tasks to fail (tests or build in other packages).

@ElianCodes
Copy link
Member

Awesome! Thanks @atilafassina! I will take a look at it when I'm back from Copenhagen, Denmark!

@matthewp
Copy link
Contributor

matthewp commented May 2, 2023

@atilafassina Saw the new commits, is this ready to go?

@atilafassina
Copy link
Contributor Author

atilafassina commented May 2, 2023

@atilafassina Saw the new commits, is this ready to go?

@matthewp , Yes. I'd say so.
Just updated and everything looks green now! :)

Copy link
Member

@ElianCodes ElianCodes left a comment

Choose a reason for hiding this comment

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

This looks awesome @atilafassina! Before this is merged, are there docs we need to adjust as well?

@atilafassina
Copy link
Contributor Author

This looks awesome @atilafassina! Before this is merged, are there docs we need to adjust as well?

@ElianCodes
I don't think so. I couldn't find any mention of the plugin not supporting SSR routes at the current docs. So I think no change is needed.

@matthewp matthewp added the semver: minor Change triggers a `minor` release label May 2, 2023
@matthewp matthewp merged commit ad90719 into withastro:main May 3, 2023
@astrobot-houston astrobot-houston mentioned this pull request May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants