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

🐛 BUG: Astro.url.pathname is missing trailing slash on prod build #4190

Closed
1 task done
oliverpool opened this issue Aug 7, 2022 · 29 comments · Fixed by #4352
Closed
1 task done

🐛 BUG: Astro.url.pathname is missing trailing slash on prod build #4190

oliverpool opened this issue Aug 7, 2022 · 29 comments · Fixed by #4352
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@oliverpool
Copy link
Contributor

oliverpool commented Aug 7, 2022

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:

Participation

  • I am willing to submit a pull request for this issue.
@oliverpool
Copy link
Contributor Author

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

@natemoo-re natemoo-re added - P4: important Violate documented behavior or significantly impacts performance (priority) s1-small labels Aug 8, 2022
@natemoo-re
Copy link
Member

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.

@oliverpool
Copy link
Contributor Author

@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.

@IanVS
Copy link
Contributor

IanVS commented Aug 15, 2022

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.

@oliverpool
Copy link
Contributor Author

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)

@IanVS
Copy link
Contributor

IanVS commented Aug 15, 2022

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

@matthewp
Copy link
Contributor

@IanVS is your issue getStaticPaths related?

@matthewp
Copy link
Contributor

@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.

@IanVS
Copy link
Contributor

IanVS commented Aug 15, 2022

@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.

@matthewp
Copy link
Contributor

@IanVS Thank you for the specifics!

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

@IanVS
Copy link
Contributor

IanVS commented Aug 15, 2022

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?

@matthewp
Copy link
Contributor

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

@IanVS
Copy link
Contributor

IanVS commented Aug 15, 2022

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

@oliverpool
Copy link
Contributor Author

@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).

@matthewp
Copy link
Contributor

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.

@IanVS
Copy link
Contributor

IanVS commented Aug 16, 2022

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

@oliverpool
Copy link
Contributor Author

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

@matthewp
Copy link
Contributor

Yeah that's an interesting point.

@matthewp
Copy link
Contributor

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.

@IanVS
Copy link
Contributor

IanVS commented Aug 16, 2022

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.

@oliverpool
Copy link
Contributor Author

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)

@IanVS
Copy link
Contributor

IanVS commented Aug 16, 2022

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. 😞

@matthewp
Copy link
Contributor

matthewp commented Aug 16, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

PR: #4352

@WuChenDi
Copy link

#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

@arlair
Copy link

arlair commented Mar 6, 2023

Am I correct that if you converted an existing site to Astro that uses essentially directory with no trailing slash, you cannot get Astro.url.pathname to remove the slash? The original idea of following trailingSlash seems to be the more sensible option for my use case.

@vincerubinetti
Copy link

vincerubinetti commented Oct 23, 2024

So, it seems like the default behavior has since been changed, because I'm getting the opposite of the behavior @IanVS describes in this comment above: in dev, presence of trailing slash matches address bar, in prod, trailing slash always there (no trailingSlash or related option set).

I get that at build time, Astro has to decide one way or the other what Astro.url.pathname will be for statically compiled pages.

But even after reading all the comments above, I still don't understand why there's a difference in behavior between dev and prod. I guess dev mode is trying to be more "smart" by generating routes/pages on the fly based on what's requested? But I'd much prefer consistent behavior between dev and prod, so I'm not caught off guard by these types of issues (another example I ran into a while back). I'd like to discover them before deployment.

I guess the fact that Astro is a framework that can deploy in many different "modes" makes this more complicated... Perhaps dev mode is running in a way that reflects the "server" mode? But doesn't Astro know which way the app is meant to be built and served ahead of time, and couldn't it match the behavior between dev and prod?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)
Projects
None yet
7 participants