Skip to content

feat: implement instanceof#149

Merged
Uzlopak merged 20 commits intomainfrom
instanceof
May 23, 2025
Merged

feat: implement instanceof#149
Uzlopak merged 20 commits intomainfrom
instanceof

Conversation

@Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented May 18, 2025

Maybe a solution for #17

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.

Works for me, LGTM.

Can you document this?

@jsumners
Copy link
Member

Does the example in #86 (comment) pass with this?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 21, 2025

@mcollina
@jsumners

What do you think about this approach?

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.

Looks good to me.

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

this approach looks great!

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 21, 2025

@mcollina
@jsumners
@gurgunday

I added some documentation. Please review again.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 22, 2025

@Fdawgs
Can you additionally to a review of the code a review of the documentation please?

@Fdawgs Fdawgs linked an issue May 22, 2025 that may be closed by this pull request
Co-authored-by: Frazer Smith <frazer.dev@icloud.com>
Signed-off-by: Aras Abbasi <aras.abbasi@googlemail.com>
@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 22, 2025

@Fdawgs

Thank you very much. I applied your suggestions :).

@jsumners
did you see the changes to the test file?

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

🚀

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 23, 2025

Should we release it as a major or as a minor?

Could be potentially better to release as major, because it is easier to track that this feature was part of v5 as v4.2.0

@Uzlopak Uzlopak merged commit 3b27b76 into main May 23, 2025
17 checks passed
@Uzlopak Uzlopak deleted the instanceof branch May 23, 2025 22:59
@Fdawgs
Copy link
Member

Fdawgs commented May 24, 2025

Should we release it as a major or as a minor?

Could be potentially better to release as major, because it is easier to track that this feature was part of v5 as v4.2.0

Minor, no breaking changes from what I can see?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 24, 2025

Just for support reasons. Like somebody asks, why it doesnt work, and then we just need to say, make npm ls and then we see, that v5 is not used everywhere, so that is the issue. Or else we need to keep in mind, that v4.2.0 is the one with this change.

Thats the only reason, i would say.

@jsumners
Copy link
Member

Trying to synchronize major version numbers across all modules to the major version of the framework is a ship that sailed a long time ago. It also introduces too many difficulties (e.g. not being able to ship a major in a module that really needs one). There's a malleable database server that forces its official modules to match the major version of the database, and it results in the modules violating semver guarantees on a routine basis. We should not do the same.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 27, 2025

@jsumners

Allright :)

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 30, 2025

@fastify/core
Should we release it?

@jsumners
Copy link
Member

Go for it.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 30, 2025

@jsumners

I dont have publish right for npm.

@jsumners
Copy link
Member

@Uzlopak please add your npm account to the new discussion I created in the private discussions. Also, since this is a library, you'd need to be on the libraries team. Are you wanting to join that team?

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.

should this package expose FastifyError?

5 participants