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

feat(gatsby-plugin-fastify): Support new trailingSlash config #147

Closed
moonmeister opened this issue Jan 28, 2022 · 5 comments
Closed

feat(gatsby-plugin-fastify): Support new trailingSlash config #147

moonmeister opened this issue Jan 28, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@moonmeister
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Gatsby now has a native config option for trailing slash config...we should support this.

gatsbyjs/gatsby#34268

@moonmeister moonmeister added the enhancement New feature or request label Jan 28, 2022
@moonmeister
Copy link
Contributor Author

gatsbyjs/gatsby#34205

@moonmeister
Copy link
Contributor Author

@moonmeister
Copy link
Contributor Author

So this went down hill...

The First Attempt

First I thought I'd just add code to the 404 Handler that returned a redirect if the given path did/didn't end in a / as determined by the config.

This generally works. But when stuff shouldn't exist it breaks down. For files...it'll appends slash (e.g. /page-data.json/). Using file extensions to detect would be possible, but also seems like a loosing battle and too much config need. Using the . in the file isn't reasonable as this is a valid character anywhere in the URL.

The other issue is sub paths...this doesn't work with client-only routes or SSR splat routes...for those we need to be able to check the actual URL and redirect based on the existence or lack there of of the final /.

This method fails cause it's both overly applied and not applied in specific cases.

The Second attempt

I began implementing this at a per plugin basis. As conversation on the spec notes...this only applies to "pages"...thus not Functions. I'll assume that applies to Redirects as well.

But That's not really true. Cause our router setting of ignoreTrailingPaths changes with our Gatsby config option. So if we changes something that should only apply to pages...it'll still affect all routes. We need to either resolve this issue or actively apply it to Functions and Redirects.

Problem 1: Static files. Because our static routing is currently handled exclusively by @fastify/static plugin which doesn't have a resolution directive capable of handling this flexibility. It has redirect which handles /test => /test/ but not the reverse.

Problem 2: Splat Routes. This issue actually currently exists. Our implementations around splat routes can't handle the situaltion where a route param is followed by a splat. Because of fastify/fastify#3331 we're currently re-writing /test/* => /test* so that it matches /test, /test/, and /test/example.... But if we palce a route param before (e.g. /test/:param/*) this becomes /test/:param* which is garbage and find-my-way can't route to it. Worse yet it doesn't complain about this format,

To me this feels like a LMW bug. It should a least throw an error. But also IMO I shold be able to pass it /test/* or /test:/:param/* and have /test and /test/example work if ignoreTrailingSlash is true. 🤷🏻

Path Forward

Okay I see to primary issues;

  1. find-my-way doesn't have the built in flexibility to handle routing as we'd like.
  2. @fastify/static doesn't contain flexibility we need to correctly server static files.
  3. Even if the router is working correctly the router can't know to handle splat routes that aren't the root with a redirect instead of a 200. Nor should it...this is an application level issue.

Potential solutions

  1. While I disagree with aspects of the find-my-way implementation...and we could work to upstream some issues...that'd take lots of time. The clearest path forward in my mind is to no rely on ignoreTrailingPaths. This fixes several long standing issues mentioned above and means we can be very specific in how we want the router to work.
  2. Other server have the concept of fall backs...e.g. if /test comes in check for test.html. /test/index.html and all that's handled separate from the fact that we should or shouldn't (or don't care) about a trailing slash. We have three options.
    1. Stop serving static files! You heard me right. I've been clear that things like compression, certs, and more all fall under the realm of "edge server" not "application server". Do static files to? That's how PHP-FPM works! I'd be configuring Caddy to serve non PHP files and Caddy proxies via PHP-FPM to serve all PHP. The same could be applied with Node. Though not all JS is executed on the server. If Caddy got a request for a file that existed...it'd serve that. IF not, it'd ask node. This isn't as clear cut as PHP, and has the down side of more hassle for the user needing to know another server and how to configure that for Gatsby static files. The more I think through this option...the less I like it.
    2. Write a custom implementation of the static plugin. We could still use it's sendFile method but write our own file lookup algorithm.
    3. Upstream a patch to the plugin to allow for this functionality. This might be a good solution...it means we only write specific code to our problem space and don't duplicate efforts. I'm guessing landing this wouldn't be a battle as we should be able to make it backwards compatible. The only potential issues is @fastify/static is implemented using send from the express world. I don't know either plugin well enough to judge which one is the culprit, and where we could fix this issue. This would require some investigation. If it's in send that could take a while if we have to update 2 levels of packages.
  3. This is solved by checking page based routes (things that this setting should affect, not functions, static files, redirects) for the existence or lac there of a a trailing slash and redirecting appropriately. Along with this we'd be registering redirect routes for only what's needed.

Remaining obstacles

Does LMW block our custom implementation for splat routes? Context, light my way handles ignoreTrailingPaths by modifying the requested URL to not include a trailing slash and probably does the same on registration. For the ignore More this is helpful, cause we don't have to register two paths with the same response code. Though I'm not sure that's actually a performance issue. If we're passing it a function, it should still only have that in memory once. We might be able to use some kind of rewrites in our custom implementation for this too. I think we can handle all this.

There are 2 kinds of static serving though (hmmmm). css, js, images, etcs (aka. assets) will be served directly...trailingSlash need not apply. But any HTML files will need to be redirected to the correct path.

Questions

  • If this is an SEO thing, should this affect rewrites?

Conclusions

I though it was going to be hard...then I started and it was getting easier...than it got hard like I didn't think possible. 😢 We'll need to watch performance and hopefully that gets better ...or not worse!

@moonmeister
Copy link
Contributor Author

With the announcement of the Gatsby Adapters spec, we'll be discontinuing any work on this plugin. Should the need for a performant node server still be needed, this work can continue under the official spec as gatsby-adapter-fastify.

@moonmeister moonmeister closed this as not planned Won't fix, can't repro, duplicate, stale Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant