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

Reorder Generic Types of FastifyReply #4415

Closed
wants to merge 1 commit into from

Conversation

sceccotti89
Copy link

@sceccotti89 sceccotti89 commented Nov 13, 2022

Issue: #4414

I didn't feel the need to write new tests since we are just reordering reply types.

Checklist

@github-actions github-actions bot added the typescript TypeScript related label Nov 13, 2022
@mcollina
Copy link
Member

What does @fastify/typescript think of this change?

@mcollina mcollina requested a review from a team November 13, 2022 12:53
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

Re-order is good, but it is not safe to land in main.

@sceccotti89
Copy link
Author

I do agree on that because it might be a breaking change. If you may suggest a different destination branch I'm more than willing to change it.

@climba03003
Copy link
Member

You can target to next branch.

@climba03003 climba03003 added semver-major Issue or PR that should land as semver major v5.x Issue or pr related to Fastify v5 labels Nov 13, 2022
@sceccotti89 sceccotti89 changed the base branch from main to next November 13, 2022 14:50
@sceccotti89
Copy link
Author

sceccotti89 commented Nov 13, 2022

Thanks @climba03003 for the feedback. Target branch changed to next.

@Uzlopak Uzlopak changed the title Change handler reply type definition Reorder Generic Types of FastifyReply Nov 14, 2022
@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 14, 2022

I changed the title to express better what this PR is doing.

@climba03003
Copy link
Member

climba03003 commented Nov 14, 2022

Can you rebase again. It included other commit in your PR.

@sceccotti89
Copy link
Author

@climba03003 I've been finally able to rebase the pull request. Sorry but I don't feel comfortable with rebasing.

@climba03003
Copy link
Member

Sorry but I don't feel comfortable with rebasing.

I usually use GUI for rebasing, command would be too complicated if you don't familiar with git.

@sceccotti89
Copy link
Author

@climba03003 I don't want to go off-topic, but I'd like to know more about the GUI you're using (is it tortoise, perhaps?). I normally use this command git rebase -i HEAD~<number> but, as you said, it's not always user friendly (especially if there has been a merge).

@climba03003
Copy link
Member

@climba03003 I don't want to go off-topic, but I'd like to know more about the GUI you're using (is it tortoise, perhaps?). I normally use this command git rebase -i HEAD~<number> but, as you said, it's not always user friendly (especially if there has been a merge).

I use git fork. You need to use --onto if you want to change the base branch.

@sceccotti89
Copy link
Author

sceccotti89 commented Nov 14, 2022

@climba03003 Thanks for the info, I'll take a look.
Btw, tests failed. According to logs it should not be related to my latest changes 🙄

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 18, 2022

@climba03003
can you please add the commits from master to next to have the latest fixes regarding node19 and tap 16.3.1 issues?

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 21, 2022

Uff, I think this also means that all plugins, which decorate fastifyReply need changes.

@mcollina mcollina deleted the branch fastify:next May 7, 2024 13:07
@mcollina mcollina closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Issue or PR that should land as semver major typescript TypeScript related v5.x Issue or pr related to Fastify v5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants