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

trailingSlash feature parity between now and serve #3731

Closed
danielstocks opened this issue Feb 4, 2020 · 5 comments · Fixed by #3745
Closed

trailingSlash feature parity between now and serve #3731

danielstocks opened this issue Feb 4, 2020 · 5 comments · Fixed by #3745

Comments

@danielstocks
Copy link

@danielstocks danielstocks commented Feb 4, 2020

Hello,

I'm not sure if this i a legit issue, but I can see that zeit-cli consumes the serve-handler package and I was under the impression that now dev used serve under the hood, but I might be out on a whim here.

The Serve README also says:

Once it's time to push your site to production, we recommend using ZEIT Now.

Anyhow, there is an inconsistency when using the trailingSlash option in now deployments. It boils down to:

  1. Now will redirect files (eg /style.css) to trailing slash. Serve does not.
  2. Now redirects with a 308 status, Serve redirects with a 301 status. (this is less of an issue just a noted difference)

I've setup a reproducible test-case here: https://github.com/danielstocks/trailing-slash-experiments.

So my question is: Is this by design or can it be considered a bug?

Thanks in advance

@danielstocks danielstocks changed the title Trailing slash feature parity between now and serve trailingSlash feature parity between now and serve Feb 4, 2020
@styfle styfle added the question label Feb 4, 2020
@styfle

This comment has been minimized.

Copy link
Member

@styfle styfle commented Feb 4, 2020

Hi @danielstocks

now dev and as well as production deployments do not use serve or serve-handler.

The expected behavior when trailingSlash: true is the following:

Visiting a path that does not end with a forward slash will respond with a 308 status code and redirect to the path with a trailing slash. See docs

There is no difference for directories or files.

I will discuss with the team if we can detect directories in production to make the behavior more nuanced but we will probably choose to keep it as-is to avoid the performance hit.

@danielstocks

This comment has been minimized.

Copy link
Author

@danielstocks danielstocks commented Feb 4, 2020

@styfle Thanks for the clarification!

And yes, to be fair now dev is actually doing what it explicitly says in the documentation. Maybe serve-handler should update it's README: https://github.com/zeit/serve-handler#trailingslash-boolean to reflect that it's treating files differently.

By default, the package will try to make assumptions for when to add trailing slashes to your URLs or not. If you want to remove them, set this property to false and true if you want to force them on all URLs

I think from a developer experience standpoint I was just confused as now is advertised as a drop-in production replacement for serve (and both are owned/maintained by Zeit Co), which given it's current API inconsistency isn't really the case.

I will discuss with the team if we can detect directories in production to make the behavior more nuanced but we will probably choose to keep it as-is to avoid the performance hit.

By looking at commits logs I've gathered that the trailingSlash configuration option is relatively new, so I don't know how many people are using it yet, or who requested it. Given that both serve, superstatic, GitHub Pages and Netlify have the "omit files" behaviour I'm a little dumbstruck as to who would even use it in it's current form. Why do it differently then everyone else?

Given the amount of stuff that is already going on in the route-handler I have a hard time imagining that using a regex to check for a file extension will have have any substantial performance regressions? After all:trailingSlash is opt-in.

Edit: updated test case to include superstatic, which has the same behaviour as serve: https://github.com/danielstocks/trailing-slash-experiments.

@danielstocks

This comment has been minimized.

Copy link
Author

@danielstocks danielstocks commented Feb 4, 2020

To circle back to why I want to redirect in the first place, here is the same page deployed with the trailingSlash option removed:

https://trailing-slash-experiments-kyyurwwr2.now.sh/test
https://trailing-slash-experiments-kyyurwwr2.now.sh/test/

As you can see, it's the same page, but it loads different CSS files depending if it has a trailing slash or not. In addition: Google et al. will treat it as as two separate pages with duplicate content.

The URL with the trailing slash makes it easier to do relative links to resources like images, css and javascript files. Because a trailing slash indicates it's a directory. That's why I prefer a trailing slash.

@styfle

This comment has been minimized.

Copy link
Member

@styfle styfle commented Feb 5, 2020

I created PR #3745 to change the behavior, thanks!

@styfle styfle added @now/routing-utils bug and removed question labels Feb 5, 2020
@danielstocks

This comment has been minimized.

Copy link
Author

@danielstocks danielstocks commented Feb 5, 2020

@styfle Well thank you! I didn’t except such a swift reply. Keep up the good work!

@styfle styfle closed this in #3745 Feb 6, 2020
styfle added a commit that referenced this issue Feb 6, 2020
#3745)

This PR changes the behavior of `trailingSlash: true` after we received feedback that files should not be redirected with a trailing slash. This matches the behavior of `serve` and `serve-handler`.

### Examples 
* `/index.html` => serve
* `/assets/style.css` => serve
* `/assets` => redirect to `/assets/`
* `/assets/style` => redirect to `/assets/style/` 

### Additional

In order to avoid duplicate content, this PR also adds redirects to files without a trailing slash.

* `/about.html/` => redirect to `/about.html`
* `/assets/style.css/` => redirect to `/assets/style.css`


Fixes #3731
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.