astro icon indicating copy to clipboard operation
astro copied to clipboard

πŸ› BUG: Astro.url.pathname is missing trailing slash on prod build

Open oliverpool opened this issue 3 years ago β€’ 3 comments

What version of astro are you using?

1.0.0-rc.7

Are you using an SSR adapter? If so, which one?

none

What package manager are you using?

yarn

What operating system are you using?

archlinux

Describe the Bug

I just upgraded to 1.0.0-rc.1 and got a warning about Astro.canonicalURL.pathname (which I am using to highlight selected menu). I replaced its usage with Astro.url.pathname and everything looked fine with astro dev.

However when doing the static build with astro build the menu does not get highlighted.

With some debugging, it turns out, that Astro.url.pathname:

  • is the exact pathname on dev (with a trailing slash if the URL has one - and not otherwise)
  • but on build there is never a trailing slash (even if the URL has one - somehow expected, since the same index.html file is served)

Looking at the (now deleted) code of createCanonicalURL https://github.com/withastro/astro/pull/3959/files#diff-7d09784396a04fa12dd4dcfbe9d6f7528bfe522487493851dc08957000481facL6 the added trailing slash has been dropped.


A proper fix would:

  • adjust this URL on dev to match the URL which would be used for build (with a trailing slash to match the behavior before RC, without if you think this would be better)
  • be documented on the migration guide

What do you think?

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-mz4j6y?file=src/pages/test.astro

Navigate to the following URLs:

  • https://github-mz4j6y--3000.local-corp.webcontainer.io/test (pathname: /test <- will be the on astro build)
  • https://github-mz4j6y--3000.local-corp.webcontainer.io/test/ (pathname: /test/)

Participation

  • [x] I am willing to submit a pull request for this issue.

oliverpool avatar Aug 07 '22 12:08 oliverpool

related to #3959 by @FredKSchott (reviewed by @matthewp and @tony-sull), who may have an opinion about this

oliverpool avatar Aug 07 '22 12:08 oliverpool

Thanks for opening an issue! This definitely sounds like a problem.

If you're willing to open a PR, we'll definitely make time to review it. Otherwise we'll try to get this prioritized as soon as we can.

natemoo-re avatar Aug 08 '22 15:08 natemoo-re

@natemoo-re thank you for the feedback.

Which solution do you think is the best?

  • add a trailing slash, when the URL should have one (not easy to implement, but similar to the behavior "pre-RC")
  • remove the trailing slash, except on root (much easier to implement, but a breaking change)

I would prefer the second option, since it is more consistent and easier to implement correctly.

oliverpool avatar Aug 08 '22 19:08 oliverpool

I just wanted to say that I hit this bug as well just now. I have some logic for my nav links that compare the pathname from the current page to the href of the link, and Percy.io noticed that my active page link was no longer being highlighted.

I think now that 1.0 has been released, the first option has to be used to avoid a breaking change, right? Otherwise, folks like me would need to go through and update the logic to remove trailing slashes, and not expect them even when they're in the url in the browser.

IanVS avatar Aug 15 '22 17:08 IanVS

avoid a breaking change

Since the current behavior (as of 1.0) is to have the paths without trailing slash on astro build, this would mean fixing the astro serve to always provide the path without trailing slash.

(so 1.0 was a breaking change and we now have to change β€œserve” to be coherent)

oliverpool avatar Aug 15 '22 17:08 oliverpool

Oh interesting, I'm still getting trailing slashes in 1.0.5...

IanVS avatar Aug 15 '22 17:08 IanVS

@IanVS is your issue getStaticPaths related?

matthewp avatar Aug 15 '22 18:08 matthewp

@oliverpool in dev Astro.url.pathname should be matching what you typed into the browser. If that is not the case then please let me know.

Build mode is trickier to answer If you have always then it should be adding them, but otherwise I'm not sure.

matthewp avatar Aug 15 '22 18:08 matthewp

@matthewp no I'm not using getStaticPaths for this. I have simply:

const currentPage = new URL(Astro.url.pathname, Astro.site).pathname;

in my astro layout's frontmatter.

On a page like http://localhost:3000/nebula/config/ the currentPage is:

  • /nebula/config in production
  • /nebula/config/ in dev

In both cases, the url is the same in the address bar. And I don't have trailingSlash set in my config.

IanVS avatar Aug 15 '22 18:08 IanVS

@IanVS Thank you for the specifics!

Correct me if I'm wrong, but does /nebula/config and /nebula/config/ both work in dev?

matthewp avatar Aug 15 '22 19:08 matthewp

In dev, the currentPage matches the url both with and without the slash. So:

Dev:

  • http://localhost:3000/nebula/config/ -> /nebula/config/
  • http://localhost:3000/nebula/config -> /nebula/config

Prod:

  • http://localhost:3000/nebula/config/ -> /nebula/config
  • http://localhost:3000/nebula/config -> /nebula/config

Is that what you were asking?

IanVS avatar Aug 15 '22 19:08 IanVS

I think so. Is the file src/pages/nebula/config.astro?

matthewp avatar Aug 15 '22 19:08 matthewp

The new URL(Astro.url.pathname, Astro.site).pathname is in the layout. But the behavior I'm seeing happens for pages created both by /src/pages/nebula/[slug].astro and /src/pages/nebula/index.astro

IanVS avatar Aug 15 '22 20:08 IanVS

@matthewp I think everything is documented into my first post (with a Minimal Reproducible Example):

dev Astro.url.pathname should be matching what you typed into the browser

It does. But my main issue is with astro build where the slash is always dropped (it is expected that it has the same value when requested with a without a trailing slash, since this is static. However it does not match the astro dev behavior).

Maybe the trailingSlash setting should be set to 'never' by default to somehow match the astro build behavior?


To sum up, my issue is that astro dev and astro build behave differently regarding Astro.(canonical)URL.pathname, which wasn't the case before 1.0.0-rc.1 (#3959 changed the logic without warning).

oliverpool avatar Aug 15 '22 20:08 oliverpool

Thanks for all of the feedback! I agree that there's a problem here, just trying to understand the nuance in order to find the correct solution.

build can only create pages once, so it has to choose either to add the trailing slash or not. Since the default trailingSlash is ignore then it doesn't know which behavior you want.

As you pointed out, Astro.canonicalURL didn't have this problem because it added a slash. Since that API is gone we are getting a behavior from Astro.url. I think the solution is that the build should probably add a trailing slash by default (if trailingSlash is not set to never) which would match our previous default.

Let me know if this would work for you @oliverpool @IanVS. I'm going to consult with the core team to see if this idea makes sense to them as well.

matthewp avatar Aug 16 '22 12:08 matthewp

Yep, that would work great on my end, thanks.

IanVS avatar Aug 16 '22 13:08 IanVS

Sounds good to me. A couple of remarks:

  • As far as I understand trailingSlash is only for the dev server. I don't see why it would impact astro build in any way. Its goal is only to match the behavior of the actual static server that will be used on production.
  • maybe the Astro.url should depend on build.format:
    • directory => with trailing slash (default)
    • file => without trailing slash

oliverpool avatar Aug 16 '22 14:08 oliverpool

Yeah that's an interesting point.

matthewp avatar Aug 16 '22 14:08 matthewp

Talked about this with core and we are not comfortable with changing the defaults now that 1.0 is out. Even though I don't like the mismatch here either, some users are likely depending on the default-no-slash behavior that currently exists.

What should be working is that if you set trailingSlash: 'always', that should force you to use a trailing slash in dev and also it should be added in the build. I'm going to write a build test to confirm that it's working, as I'm not sure it currently is.

Also the docs will be updated because although they say the config is for the dev server, it's a top-level config and should be applied to both dev and build.

matthewp avatar Aug 16 '22 14:08 matthewp

If that's the case, it might also be good to note that the replacement of Astro.url for cannonicalURL is not a direct replacement, and this difference must be accounted for when changing from one to the other.

IanVS avatar Aug 16 '22 14:08 IanVS

we are not comfortable with changing the defaults now that 1.0 is out

Too bad that this issue got attention after the 1.0 release (this issue was opened 2 days before the release).

If you don't want to change it, it means that you consider the current behavior to be working as expected? (for me the current behavior is broken)

oliverpool avatar Aug 16 '22 14:08 oliverpool

There were a lot of changes between the late betas and the final release, which went very quickly. I'm really surprised how many breaking changes were made late in the game. Definitely a bummer that there wasn't more time for community feedback. 😞

IanVS avatar Aug 16 '22 14:08 IanVS

To be fair, I don't think there's any breaking change here. Astro.url was a new feature and as far as I'm aware, we didn't change its behavior. It's just not as clean of a replacement for Astro.canonicalURL as we had thought.

The mistake, to me, is making ignore the default for trailingSlash. The build has to either have a trailing slash or not have one, we can't "ignore" it, so this creates an unfortunate dev/build difference.

We can and should fix this in 2.0. I'm not closing this issue until we have that figured out. A 2.0 is not necessarily a long way out.

I do take all feedback about the instability in the beta/RC period. We didn't do a great job of planning on breaking changes and then make them all at once; instead we allowed ourselves to continue to find things we wanted to change until close to the 1.0 release. We've heard this feedback from a lot of the community (and among ourselves) and will work to not do this the next time around.

Part of that is not breaking apps now, though. So as much as I personally do not like the current behavior, breaking it for everyone else today is not the right answer. I think the right answer is to make sure trailingSlash: 'always' works everywhere, and possibly that is what we should encourage people to use.

matthewp avatar Aug 16 '22 16:08 matthewp

Ok, the team has been talking about this again and now I think our opinion is changing based on the build.format argument you made @oliverpool. We are using this information to determine where to place the file but are not using it to determine the request URL and that feels wrong. Since you've chosen directory you should be able to resolve relative URLs as those you are within a directory. We are still talking about it but are coming around. cc @IanVS

matthewp avatar Aug 16 '22 17:08 matthewp

Ok, we do have consensus now. We will make the build follow build.format so that if it is set to directory (the default), your url will be /page/ and if it is set to file it will be page.html. This should make relative URLs created using Astro.url work as you would want/expect.

Thanks to you both @oliverpool @IanVS for making a strong case here and being patient with me. I hope to get this into a patch fix soon.

matthewp avatar Aug 16 '22 18:08 matthewp

PR: https://github.com/withastro/astro/pull/4352

matthewp avatar Aug 16 '22 18:08 matthewp

https://github.com/withastro/astro/issues/4404 @FredKSchott Thanks for your reply, I tried this configuration, but I didn't get the results I wanted.

My current problem is that when reading the astro.config.mjs file, the jump in the link component gives an extra / result, but not this.

image image image image

WuChenDi avatar Aug 22 '22 01:08 WuChenDi