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: Add redirect option #93

Merged
merged 4 commits into from
Feb 10, 2019
Merged

feat: Add redirect option #93

merged 4 commits into from
Feb 10, 2019

Conversation

coreyfarrell
Copy link
Contributor

This adds an option to redirect requests for directories when the
tailing slash is missing.

Fixes #92

This adds an option to redirect requests for directories when the
tailing slash is missing.

Fixes #92
README.md Outdated Show resolved Hide resolved
* Support `redirect: true` for folders which provide an index file.
* Support the `index` option.
index: 'index.html'
}

const fastify = Fastify()
Copy link
Member

Choose a reason for hiding this comment

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

Can you add another test with ignoreTrailingSlash: true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested redirect: true, wildcard: true with ignoreTrailingSlash: true on the server and it worked as expected without any further changes.

As is redirect: true, wildcard: false cannot be used on a server with ignoreTrailingSlash: true. To fix this would require a way to determine if ignoreTrailingSlash is enabled. In my working copy I edited node_modules/fastify/fastify.js so that the server object is created with:

  const fastify = {
    ignoreTrailingSlash: options.ignoreTrailingSlash,
    [kChildren]: []
  }

Then I made my local copy of fastify-static check if fastify.ignoreTrailingSlash is enabled to know how to register routes. The problem is that fastify.get('/static/') creates two routes so wildcard: false needs to know to not also setup fastify.get('/static'), as it stands find-my-way throws an assertion. I'd prefer to not use a try / catch block to suppress this assertion for risk of potentially suppressing errors.

An alternative would be for fastify-static to register fastify.get(pathname.replace(/\/$/, '')) before fastify.get(pathname), and fix find-my-way to only register the ignoreTrailingSlash route if it doesn't already exist. This would probably be cleaner for fastify-static.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please document the behavior that is not supported? We might want to implement this in a follow-up a PR if there is a need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I looked at find-my-route again, it appears that /path registers /path/ and /path/ registers /path. So probably the best fix would be to honor ignoreTrailingSlash: false option for find-my-way routes to override the global option, that way we could pass ignoreTrailingSlash: false for wildcard: false, redirect: true.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit fabd57b into fastify:master Feb 10, 2019
@coreyfarrell coreyfarrell deleted the directory-redirect branch February 10, 2019 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants