feat(node): add trailingSlash support #9080

msxdan · 2023-11-13T10:59:48Z

Changes

Add logic to support trailingSlash with ignore, always and never values with nodejs adapter
Closes Bare paths redirect to paths with a trailing slash in production #7808

Testing

Add tests to use each possible value of trailingSlash and check if expected results are good and not breaking other tests

trailingSlash = always → Should always contain the / at the end (will always return host/pathname/?queryifexists)
trailingSlash = never → Should never contain the / at the end (will always return host/pathname?queryifexists)
trailingSlash = ignore → Should ignore the / at the end (will always work without redirect)

New behaviour of trailingSlash = ignore will return 200 and the content of the index.html inside the directory

I had to change the prerenderer.test.js to support this new behaviour and not breaking the test

Docs

Should work as expected in current documentation

I know there's currently an open PR (#8593) about this, but I was not able to make it working and needed this for a personal project

changeset-bot · 2023-11-13T10:59:53Z

🦋 Changeset detected

Latest commit: 9f21d46

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

natemoo-re

Thank you so much for the PR! This is great work but I'm not sure if it's implemented at the correct layer. I think this might make more sense implemented in the Middleware itself? @matthewp might have the most context here.

natemoo-re · 2023-11-16T20:22:16Z

packages/integrations/node/src/http-server.ts

+	handler: http.RequestListener,
+	trailingSlash: AstroUserConfig['trailingSlash']


I'm not sure that createServer is the right place to handle this... Seems like we should be able to read the trailingSlash value from the manifest and use that to point requests to the correct route.

Yeah sounds right. What I'm unsure about is why this needs to happen in the adapter level and not inside of Astro core itself. It seems like we should be handling trailingSlash ourselves. I can't recall why we do not.

It might be the case that we forgot to initially, and thought it would be backwards incompatible to do so. I do have some recollection of handle trailing slash for things like assets at build time (by making sure it was correct in the manifest).

If that's the case, then I thinking having in the Node adapter is ok, and if we ever fix it in core then it will just already be fixed before it gets there.

In any case, fixing it inside of the middleware seems ideal.

natemoo-re · 2023-11-16T20:23:31Z

packages/integrations/node/src/types.ts

@@ -15,6 +16,7 @@ export interface Options extends UserOptions {
 	port: number;
 	server: string;
 	client: string;
+	trailingSlash: AstroUserConfig['trailingSlash'];


I don't think we should change the signature of the Node options. This information is already available in the Astro config, so it should be available to the app that is generated by Astro.

Yeah, agree here

packages/integrations/node/test/trailing-slash.js

ematipico · 2023-11-16T21:38:16Z

I wonder if this behaviour should be implemented inside astro/app. The class has already that information - this.#manifest.trailingSlash - and we could add the logic inside the match function.

florian-lefebvre · 2023-12-08T08:05:01Z

matthewp confirmed that what ematipico suggested is the way to do. Would you be willing to resolve conflicts and make the changes required @msxdan?

msxdan · 2023-12-09T10:29:39Z

matthewp confirmed that what ematipico suggested is the way to do. Would you be willing to resolve conflicts and make the changes required @msxdan?

Sure, I'll review it and make the changes suggested

msxdan · 2023-12-09T15:15:32Z

@matthewp @ematipico I'm unsure this can be handled inside the astro/app nor the middleware, when the server is created, the listener will serve all requests unless there's an error, when an error is reached it will then reach the SSR Handler (the middleware) and call the match method inside NodeApp

http-server.ts

Should we call the handler for each request? that way it will call the match function everytime, however I'm not sure that will work fine, the standalone mode is serving the content by itself unless there's an error

matthewp · 2023-12-11T15:40:56Z

@msxdan I'm not sure I understand, that stream code is about serving files. It checks if there's a file and if not, it calls the SSR handler. But I'm not sure what this has to do with trailingSlash. Aren't we already calling into astro/app for the trailing slash, but astro/app is just not handling it?

msxdan · 2023-12-12T09:50:18Z

@matthewp The astro/app was not called by default before serving the file, only when there's no existing file, I cannot add the trailingSlash logic inside match function if it's not called.

If you can tell me some suggestions about how it should work I could try to implement it.

matthewp · 2023-12-14T13:49:02Z

@msxdan That's how it's supposed to work, it checks for files first, and if no files exist, it then calls Astro to render. Are you expecting Astro render to take precedent over files?

florian-lefebvre · 2024-01-05T08:50:41Z

@msxdan are you still interested in this PR?

msxdan · 2024-01-05T09:01:32Z

@msxdan are you still interested in this PR?

Yes I am, sorry I've been busy all these days

msxdan · 2024-01-07T19:12:03Z

@msxdan That's how it's supposed to work, it checks for files first, and if no files exist, it then calls Astro to render. Are you expecting Astro render to take precedent over files?

@matthewp about our last conversation

The static files are served from the http-server and we have no context about the manifest and hence the trailingSlash, that's why I added the trailingSlash inside Options

I tried to use the render function inside the astro core but it is unable to render static files or I don't know how it works, so since the App or NodeApp has the trailingSlash info I don't know how to implement it inside the http-server, and the nodeMidleware cannot render the files so I'm a bit stuck

matthewp · 2024-01-17T15:51:10Z

@msxdan Let's use an example to talk through. Let's say you have these files:

public/penguin.png
src/pages/[...page].astro

If page renders were prioritized then it would be impossible to ever get the image at /penguin.png. That's why we check static files first.

If you'd like to change this to make files be checked last, we can talk about doing that through a new option perhaps, but that's different from getting trailingSlash to work.

Could create a reproduction with astro.new that shows trailingSlash: ignore not working?

detected an infinite loop in base path when trailingSlash: never

lilnasy · 2024-01-18T19:50:16Z

packages/integrations/node/src/http-server.ts

+			if (urlQuery && !pathname.includes('?')) {
+				pathname = pathname + '?' + urlQuery;
+			}


Can you elaborate on this line?

If we add the query right before send handles the pathname, wouldn't it prevent send from matching files?

@msxdan I would be ready to approve after clarification on this

@lilnasy I'm not sure but I guess if it was failing the tests wouldn't be passing, isn't it?, I think the query data should be included in the pathname since if you do a redirect without it you'll lose the query data

with trailingSlash: always

domain.com/test?name=astro -> 301 domain.com/test/?name=astro

After my rebase, it was failing the tests with it.

That's a good point. I hadn't thought of it, but that's behavior I would want as well.

In the code, I see that the query data is added to the path right before performing a redirect. I see that you had written tests for it as well.

So, it looks good to me if it looks good to you.

Just checked, looks good to me as well!

lilnasy · 2024-01-18T19:55:57Z

The node adapter just had a major overhaul. I have rebased this PR on top of the changes introduced.

Original commits:

feat(node): add trailing slash support
add changeset
test(node): add base route test in trailing-slash.js
detected an infinite loop in base path when trailingSlash: never
fix(node): avoid infinite redirect when trailingSlash: never

lilnasy · 2024-01-18T20:42:45Z

!preview node-trailing-slash

github-actions · 2024-01-18T20:44:35Z

Snapshots have been released for the following packages:

@astrojs/node@experimental--node-trailing-slash

Publish Log

🦋  warn ===============================IMPORTANT!===============================
🦋  warn Packages will be released under the experimental--node-trailing-slash tag
🦋  warn ----------------------------------------------------------------------
🦋  info npm info astro
🦋  info npm info @astrojs/prism
🦋  info npm info @astrojs/rss
🦋  info npm info create-astro
🦋  info npm info @astrojs/alpinejs
🦋  info npm info @astrojs/lit
🦋  info npm info @astrojs/markdoc
🦋  info npm info @astrojs/mdx
🦋  info npm info @astrojs/node
🦋  info npm info @astrojs/partytown
🦋  info npm info @astrojs/preact
🦋  info npm info @astrojs/react
🦋  info npm info @astrojs/sitemap
🦋  info npm info @astrojs/solid-js
🦋  info npm info @astrojs/svelte
🦋  info npm info @astrojs/tailwind
🦋  info npm info @astrojs/vercel
🦋  info npm info @astrojs/vue
🦋  info npm info @astrojs/internal-helpers
🦋  info npm info @astrojs/markdown-remark
🦋  info npm info @astrojs/telemetry
🦋  info npm info @astrojs/underscore-redirects
🦋  info npm info @astrojs/upgrade
🦋  warn astro is not being published because version 4.2.1 is already published on npm
🦋  warn @astrojs/prism is not being published because version 3.0.0 is already published on npm
🦋  warn @astrojs/rss is not being published because version 4.0.2 is already published on npm
🦋  warn create-astro is not being published because version 4.7.1 is already published on npm
🦋  warn @astrojs/alpinejs is not being published because version 0.3.2 is already published on npm
🦋  warn @astrojs/lit is not being published because version 4.0.1 is already published on npm
🦋  warn @astrojs/markdoc is not being published because version 0.8.3 is already published on npm
🦋  warn @astrojs/mdx is not being published because version 2.0.5 is already published on npm
🦋  info @astrojs/node is being published because our local version (0.0.0-node-trailing-slash-20240118204342) has not been published on npm
🦋  warn @astrojs/partytown is not being published because version 2.0.4 is already published on npm
🦋  warn @astrojs/preact is not being published because version 3.1.0 is already published on npm
🦋  warn @astrojs/react is not being published because version 3.0.9 is already published on npm
🦋  warn @astrojs/sitemap is not being published because version 3.0.5 is already published on npm
🦋  warn @astrojs/solid-js is not being published because version 4.0.1 is already published on npm
🦋  warn @astrojs/svelte is not being published because version 5.0.3 is already published on npm
🦋  warn @astrojs/tailwind is not being published because version 5.1.0 is already published on npm
🦋  warn @astrojs/vercel is not being published because version 7.0.0 is already published on npm
🦋  warn @astrojs/vue is not being published because version 4.0.8 is already published on npm
🦋  warn @astrojs/internal-helpers is not being published because version 0.2.1 is already published on npm
🦋  warn @astrojs/markdown-remark is not being published because version 4.1.0 is already published on npm
🦋  warn @astrojs/telemetry is not being published because version 3.0.4 is already published on npm
🦋  warn @astrojs/underscore-redirects is not being published because version 0.3.3 is already published on npm
🦋  warn @astrojs/upgrade is not being published because version 0.2.2 is already published on npm
🦋  info Publishing "@astrojs/node" at "0.0.0-node-trailing-slash-20240118204342"
🦋  success packages published successfully:
🦋  @astrojs/node@0.0.0-node-trailing-slash-20240118204342
🦋  Creating git tag...
🦋  New tag:  @astrojs/node@0.0.0-node-trailing-slash-20240118204342

Build Log


> root@0.0.0 build /home/runner/work/astro/astro
> turbo run build --filter=astro --filter=create-astro --filter="@astrojs/*" --filter="@benchmark/*"

• Packages in scope: @astrojs/alpinejs, @astrojs/cloudflare, @astrojs/internal-helpers, @astrojs/lit, @astrojs/markdoc, @astrojs/markdown-remark, @astrojs/mdx, @astrojs/netlify, @astrojs/node, @astrojs/partytown, @astrojs/preact, @astrojs/prism, @astrojs/react, @astrojs/rss, @astrojs/sitemap, @astrojs/solid-js, @astrojs/svelte, @astrojs/tailwind, @astrojs/telemetry, @astrojs/underscore-redirects, @astrojs/upgrade, @astrojs/vercel, @astrojs/vue, @benchmark/timer, astro, create-astro
• Running build in 26 packages
• Remote caching enabled
::group::@astrojs/telemetry:build
cache hit, suppressing logs 0d2dc72558948141
::endgroup::
::group::create-astro:build
cache hit, suppressing logs 4a08e72b37b63a3b
::endgroup::
::group::@astrojs/internal-helpers:build
cache hit, suppressing logs 737aec08a2a58880
::endgroup::
::group::@astrojs/upgrade:build
cache hit, suppressing logs fb108792d4e68d07
::endgroup::
::group::@astrojs/prism:build
cache hit, suppressing logs a175f0e52eea6903
::endgroup::
::group::@astrojs/markdown-remark:build
cache miss, executing ced04dae14f0001d

> @astrojs/markdown-remark@4.1.0 build /home/runner/work/astro/astro/packages/markdown/remark
> astro-scripts build "src/**/*.ts" && tsc -p tsconfig.json

::endgroup::
::group::astro:build
cache miss, executing 87492b455f6c4902

> astro@4.2.1 build /home/runner/work/astro/astro/packages/astro
> pnpm run prebuild && astro-scripts build "src/**/*.{ts,js}" && tsc && pnpm run postbuild


> astro@4.2.1 prebuild /home/runner/work/astro/astro/packages/astro
> astro-scripts prebuild --to-string "src/runtime/server/astro-island.ts" "src/runtime/client/{idle,load,media,only,visible}.ts"


> astro@4.2.1 postbuild /home/runner/work/astro/astro/packages/astro
> astro-scripts copy "src/**/*.astro" && astro-scripts copy "src/**/*.wasm"

::endgroup::
::group::@astrojs/partytown:build
cache miss, executing a1d55291531f99ea

> @astrojs/partytown@2.0.4 build /home/runner/work/astro/astro/packages/integrations/partytown
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/underscore-redirects:build
cache miss, executing 4257aa907833460c

> @astrojs/underscore-redirects@0.3.3 build /home/runner/work/astro/astro/packages/underscore-redirects
> astro-scripts build "src/**/*.ts" && tsc -p tsconfig.json

::endgroup::
::group::@astrojs/react:build
cache miss, executing 815b27da1c804a0e

> @astrojs/react@3.0.9 build /home/runner/work/astro/astro/packages/integrations/react
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/rss:build
cache miss, executing 0cd6f4eef7d46a3f

> @astrojs/rss@4.0.2 build /home/runner/work/astro/astro/packages/astro-rss
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/tailwind:build
cache miss, executing cae53cbf3b33481b

> @astrojs/tailwind@5.1.0 build /home/runner/work/astro/astro/packages/integrations/tailwind
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/solid-js:build
cache miss, executing 101eb742c87c7b46

> @astrojs/solid-js@4.0.1 build /home/runner/work/astro/astro/packages/integrations/solid
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/vercel:build
cache miss, executing 1a5ac8cac6a7c71a

> @astrojs/vercel@7.0.0 build /home/runner/work/astro/astro/packages/integrations/vercel
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/mdx:build
cache miss, executing e0bdbedce33dc1f4

> @astrojs/mdx@2.0.5 build /home/runner/work/astro/astro/packages/integrations/mdx
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/markdoc:build
cache miss, executing 135438adafd15863

> @astrojs/markdoc@0.8.3 build /home/runner/work/astro/astro/packages/integrations/markdoc
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/preact:build
cache miss, executing 754c5a1a4f71acb5

> @astrojs/preact@3.1.0 build /home/runner/work/astro/astro/packages/integrations/preact
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/lit:build
cache miss, executing 2235138c931bfa0e

> @astrojs/lit@4.0.1 build /home/runner/work/astro/astro/packages/integrations/lit
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/alpinejs:build
cache miss, executing 176179ae712eec31

> @astrojs/alpinejs@0.3.2 build /home/runner/work/astro/astro/packages/integrations/alpinejs
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/node:build
cache miss, executing 1fd5d154ffeff3c1

> @astrojs/node@0.0.0-node-trailing-slash-20240118204342 build /home/runner/work/astro/astro/packages/integrations/node
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@benchmark/timer:build
cache miss, executing 553580bb7c516289

> @benchmark/timer@0.0.0 build /home/runner/work/astro/astro/benchmark/packages/timer
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/svelte:build
cache miss, executing 2e6a27b8b3118038

> @astrojs/svelte@5.0.3 build /home/runner/work/astro/astro/packages/integrations/svelte
> astro-scripts build "src/index.ts" && astro-scripts build "src/editor.cts" --force-cjs --no-clean-dist && tsc

::endgroup::
::group::@astrojs/vue:build
cache miss, executing ee86577553f85f4a

> @astrojs/vue@4.0.8 build /home/runner/work/astro/astro/packages/integrations/vue
> astro-scripts build "src/index.ts" && astro-scripts build "src/editor.cts" --force-cjs --no-clean-dist && tsc

::endgroup::
::group::@astrojs/sitemap:build
cache miss, executing 842d23b0ca1bee9d

> @astrojs/sitemap@3.0.5 build /home/runner/work/astro/astro/packages/integrations/sitemap
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::

 Tasks:    24 successful, 24 total
Cached:    5 cached, 24 total
  Time:    41.761s

ematipico · 2024-01-19T13:59:36Z

@lilnasy

I did some testing:

In prod (astro preview / node ./dist/server/entry.mjs) it works as expected
In dev, there are still issues. The dev server keeps adding a trailing slash regardless of the option passed via configuration

lilnasy

Looks great to me! Thanks for the clean fix and thorough tests.

ematipico

Happy to merge too. After more testing, I could see things working as expected

msxdan changed the title ~~feat(node): add trailingSlash supprt~~ feat(node): add trailingSlash support Nov 13, 2023

github-actions bot added pkg: integration Related to any renderer integration (scope) pr: docs A PR that includes documentation for review labels Nov 13, 2023

natemoo-re reviewed Nov 16, 2023

View reviewed changes

natemoo-re requested a review from matthewp November 16, 2023 20:28

florian-lefebvre mentioned this pull request Dec 15, 2023

fix: Bare paths redirect to paths with a trailing slash in production #8593

Closed

kamranahmedse mentioned this pull request Dec 24, 2023

Node adapter does not support build.format: "file" for prerendered pages #9128

Closed

1 task

bluwy linked an issue Dec 26, 2023 that may be closed by this pull request

Node adapter does not support build.format: "file" for prerendered pages #9128

Closed

1 task

This comment was marked as outdated.

Sign in to view

ematipico modified the milestone: 4.2.0 Jan 4, 2024

sarah11918 removed the pr: docs A PR that includes documentation for review label Jan 9, 2024

msxdan and others added 4 commits January 18, 2024 19:29

feat(node): add trailing slash support

30467d8

add changeset

d408724

test(node): add base route test in trailing-slash.js

c0e4e01

detected an infinite loop in base path when trailingSlash: never

fix(node): avoid infinite redirect when trailingSlash: never

3252a2b

lilnasy reviewed Jan 18, 2024

View reviewed changes

lilnasy force-pushed the feat/node-trailing-slash branch from 0a69da8 to 55f0468 Compare January 18, 2024 19:53

lilnasy added 2 commits January 18, 2024 20:21

address test failures after rebase pt.1

7637ce9

address test failures after rebase pt.2

9f21d46

lilnasy force-pushed the feat/node-trailing-slash branch from 55f0468 to 9f21d46 Compare January 18, 2024 20:34

lilnasy approved these changes Jan 24, 2024

View reviewed changes

ematipico approved these changes Jan 25, 2024

View reviewed changes

ematipico merged commit a12196d into withastro:main Jan 25, 2024
13 checks passed

astrobot-houston mentioned this pull request Jan 25, 2024

[ci] release #9802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node): add trailingSlash support #9080

feat(node): add trailingSlash support #9080

msxdan commented Nov 13, 2023

changeset-bot bot commented Nov 13, 2023 •

edited

Loading

natemoo-re left a comment

natemoo-re Nov 16, 2023

matthewp Nov 16, 2023

natemoo-re Nov 16, 2023

matthewp Nov 16, 2023

ematipico commented Nov 16, 2023 •

edited

Loading

florian-lefebvre commented Dec 8, 2023

msxdan commented Dec 9, 2023

msxdan commented Dec 9, 2023

matthewp commented Dec 11, 2023

msxdan commented Dec 12, 2023

matthewp commented Dec 14, 2023

This comment was marked as outdated.

florian-lefebvre commented Jan 5, 2024

msxdan commented Jan 5, 2024

msxdan commented Jan 7, 2024

matthewp commented Jan 17, 2024

lilnasy Jan 18, 2024

lilnasy Jan 24, 2024

msxdan Jan 24, 2024

lilnasy Jan 24, 2024

lilnasy Jan 24, 2024

msxdan Jan 24, 2024

lilnasy commented Jan 18, 2024

lilnasy commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

ematipico commented Jan 19, 2024

lilnasy left a comment

ematipico left a comment

		handler: http.RequestListener,
		trailingSlash: AstroUserConfig['trailingSlash']

feat(node): add trailingSlash support #9080

feat(node): add trailingSlash support #9080

Conversation

msxdan commented Nov 13, 2023

Changes

Testing

Docs

changeset-bot bot commented Nov 13, 2023 • edited Loading

🦋 Changeset detected

natemoo-re left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Nov 16, 2023 • edited Loading

florian-lefebvre commented Dec 8, 2023

msxdan commented Dec 9, 2023

msxdan commented Dec 9, 2023

matthewp commented Dec 11, 2023

msxdan commented Dec 12, 2023

matthewp commented Dec 14, 2023

This comment was marked as outdated.

florian-lefebvre commented Jan 5, 2024

msxdan commented Jan 5, 2024

msxdan commented Jan 7, 2024

matthewp commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilnasy commented Jan 18, 2024

lilnasy commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

ematipico commented Jan 19, 2024

lilnasy left a comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Nov 13, 2023 •

edited

Loading

ematipico commented Nov 16, 2023 •

edited

Loading