Skip to content

Reorder Generic Types of FastifyReply#4415

Closed
sceccotti89 wants to merge 1 commit intofastify:nextfrom
sceccotti89:feat/4414
Closed

Reorder Generic Types of FastifyReply#4415
sceccotti89 wants to merge 1 commit intofastify:nextfrom
sceccotti89:feat/4414

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.

@github-actions github-actions bot removed the typescript TypeScript related label Nov 14, 2022
@github-actions github-actions bot added the typescript TypeScript related label Nov 14, 2022
@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
@github-actions
Copy link

github-actions bot commented May 8, 2025

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 May 8, 2025
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 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.

4 participants