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

Set exposeHeadRoutes: true by default #2826

Merged
merged 4 commits into from Feb 2, 2021
Merged

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Jan 31, 2021

Checklist

lib/route.js Outdated
const config = opts.config || {}
config.url = url
config.method = opts.method
const config = Object.assign({ url, method: opts.method }, opts.config)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bad bug that's present on master as well.

Copy link
Member

Choose a reason for hiding this comment

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

Nice finding!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in master with #2827

@mcollina
Copy link
Member Author

I took the lazy route and disabled the HEAD routes in certain tests. Updating them would have made more complex to read from my point of view.

@mcollina mcollina marked this pull request as ready for review January 31, 2021 12:10
fastify.js Outdated Show resolved Hide resolved
Copy link
Member

@L2jLiga L2jLiga left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jsumners jsumners 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
Copy link
Member Author

mcollina commented Feb 2, 2021

PTAL, I've rebased.

Copy link
Member

@salmanm salmanm left a comment

Choose a reason for hiding this comment

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

lgtm.
Just for my curiosity and since there isn't any issue linked, would like to know what's the motivation behind it?

@zekth
Copy link
Member

zekth commented Feb 2, 2021

lgtm.
Just for my curiosity and since there isn't any issue linked, would like to know what's the motivation behind it?

Following this one: #2700

@mcollina mcollina merged commit 1003d9d into next Feb 2, 2021
@mcollina mcollina deleted the expose-head-by-default branch February 2, 2021 14:15
@mcollina mcollina added semver-major Issue or PR that should land as semver major v4.x Issue or pr related to Fastify v4 labels Feb 2, 2021
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-major Issue or PR that should land as semver major v4.x Issue or pr related to Fastify v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants