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

Make Astro.url conform to build.format during the build #4352

Merged
merged 7 commits into from
Aug 25, 2022

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Aug 16, 2022

Changes

  • Makes the Astro.url.pathname match what configuration is set to in build.format.
  • This means that if the config is set to 'directory' it will be /page/. If it is set to 'file' it will be /page.html.
  • This makes it so that relative URLs you create using Astro.url resolve the way your configuration specifies.
  • Fixes 🐛 BUG: Astro.url.pathname is missing trailing slash on prod build #4190

Testing

  • Test added for both 'directory' and 'file'.

Docs

  • Included in this PR in the astro.ts types file.

@changeset-bot
Copy link

changeset-bot bot commented Aug 16, 2022

🦋 Changeset detected

Latest commit: bfb9ba6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
astro Minor
@e2e/astro-component Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/hydration-race Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/solid-recurse Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch

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 the pkg: astro Related to the core `astro` package (scope) label Aug 16, 2022
@oliverpool
Copy link
Contributor

Thanks for this PR!

I think it is worth a documentation update:

  • trailingslash mentions buildOptions.pageUrlFormat in "See Also" => it should be build.format
  • format => a mention that this will affect Astro.url should be added (otherwise the build could be subtly different).

@matthewp
Copy link
Contributor Author

Good call-out, this change needs docs updates as well.

@matthewp matthewp marked this pull request as draft August 16, 2022 19:43
@oliverpool
Copy link
Contributor

Stupid question: is there a simple way for me to test this branch on my project?

I tried using gitpkg.now.sh, as recommended on SO, but it failed with an error 500
npm install 'https://gitpkg.now.sh/withastro/astro/packages/astro?fix-astro-url-page-format'

@matthewp
Copy link
Contributor Author

There's not really a good way. Some times we've done PR releases but I'm not sure how involved that is.

@matthewp
Copy link
Contributor Author

I was able to create a preview release, so you should be able to install with npm install astro@next--format-astro-url

@delucis
Copy link
Member

delucis commented Aug 16, 2022

The associated docs PR for this (withastro/docs#1321) actually needs to target the @types/astro.ts file in this repo, so do you want to move those changes into this PR?

@oliverpool
Copy link
Contributor

oliverpool commented Aug 17, 2022

Using your package 0.0.0-20220816201344 (thank you very much for this!!!!!) here is what I go:

  • the astro build behaves as expected in both file and directory cases (for Astro.url & Astro.request.url) 👍
  • the astro dev is still depending on the actual browser url:
    • http://localhost:3000/termine/ => Astro.url.pathname == "/termine/"
    • http://localhost:3000/termine => Astro.url.pathname == "/termine" (menu does not get highlighted 😕 )

Both URLs reply, since I use trailingSlash:"ignore" (which is the default).

@matthewp
Copy link
Contributor Author

Yep, ignore means ignore. It means you want to support both URL patterns. To do this you need to account for the fact that Astro.url might have a trailing slash or might not.

@oliverpool
Copy link
Contributor

My main issue, is that it will behave differently when deployed.

If I use the php server php -S localhost:8090 -t dist, both /termine/ and /termine serve the file /termine/index.html (with Astro.url having the trailing slash).
How can I match this setup with astro dev?


I just checked, and my actual hosting (netlify) redirects /termine/ to /termine.
Maybe this would be a nice option addition to trailingSlash: redirect. Shall I open a feature request?

@matthewp
Copy link
Contributor Author

@oliverpool I don't know if we should reuse this config option to do redirects (maybe!) but the request for some solution is very reasonable. The place to talk about this would be in a discussion here: https://github.com/withastro/rfcs/discussions

@matthewp matthewp marked this pull request as ready for review August 22, 2022 16:04
Copy link
Contributor

@tony-sull tony-sull left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I've been following the discussion of how trailing slash will work and this makes sense to me 👍

@matthewp matthewp added the semver: minor Change triggers a `minor` release label Aug 25, 2022
@matthewp matthewp merged commit cd154e4 into main Aug 25, 2022
@matthewp matthewp deleted the fix-astro-url-page-format branch August 25, 2022 19:21
@GrantSmithDoddle
Copy link

Sorry if I'm not following, is this issue fixed?

As I am seeing this exact behaviour.

I'm using const canonicalURL = Astro.props.canonicalURL || new URL(Astro.url.pathname, Astro.site);

On running npm run dev, canonicalURL = https://website.com/page
On running npm run build, canonicalURL = https://website.com/page/

My config has:

site: 'https://website.com',
output: 'static',
trailingSlash: 'never'

I'm running "astro": "^2.0.2"

@oliverpool
Copy link
Contributor

@GrantSmithDoddle which page do you visit on npm run dev?

http://localhost:3000/page or
http://localhost:3000/page/ ?

You should probably set trailingSlash: 'always'

I mentioned this shortcoming in #4352 (comment), but AFAIK nothing has been done to prevent this inconsistent behavior...

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) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: Astro.url.pathname is missing trailing slash on prod build
6 participants