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

fix: allow assigning provider-enabled instances to FastifyInstance #43

Merged
merged 2 commits into from
Nov 16, 2022

Conversation

driimus
Copy link
Contributor

@driimus driimus commented Oct 24, 2022

Checklist

Fix for fastify/fastify#4342 specific to this type provider.

@driimus driimus changed the title fix: allow assigning to FastifyInstance fix: allow assigning provider-enabled instances to FastifyInstance Oct 24, 2022
@driimus driimus marked this pull request as ready for review October 27, 2022 08:24
@sinclairzx81
Copy link
Contributor

@RafaelGSS Hi, just noticed this PR was still pending.

I think it should be ok to merge this one through as the type specs were updated on fastify/fastify#4371 in preparation for this. The original PR provided adequate testing when infering unknown for unresolvable types (currently the providers infer as never). So this PR would be the subsequent downstream update inline with the main project specs (see here)

There's also a similar PR ready to go through for fastify/fastify-type-provider-json-schema-to-ts#21 package which implements the same update as this provider.

So just as a quick reminder on this. This update relates specifically to enabling contravariance of the FastifyInstance when configured with a Type Provider. This should help greatly improve type composability and assignment of FastifyInstance (as it makes the configured provider instance assignable to non configured instances, which is awesome)

I don't anticipate this change causing any problems in existing codebases currently using this or the json-schema provider. But may want to use a minor revision just in case.

Would be a great to see this PR merged through!

@RafaelGSS
Copy link
Member

@driimus can you rebase?

@driimus
Copy link
Contributor Author

driimus commented Nov 15, 2022

@RafaelGSS the PR is already up-to-date with main. Happy to rebase onto another ref if you have one in mind.

If it's about the failing CI for node14/windows, that shouldn't be related to the changes in this PR.

It sounds more like tapjs/tapjs#843. The project doesn't have a lockfile so the version of tap that gets installed is newer than the one in package.json:

I've created a clone of the repo to run the CI workflow on main: https://github.com/driimus/ff-type-provier-private/actions/runs/3473691689
Potential fixes:

Unless the tap issue (or npm/cli#5856) is resolved quickly, there might be a need to coordinate the fix across repos in the fastify org.

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@RafaelGSS RafaelGSS merged commit 807e5b2 into fastify:main Nov 16, 2022
@RafaelGSS
Copy link
Member

I think the most appropriate is to release the minor when the tap issue is resolved.

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.

None yet

4 participants