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

build(deps): revert to peerDependency, refer to #73 #78

Merged
merged 3 commits into from
May 2, 2023

Conversation

nadhifikbarw
Copy link
Contributor

Checklist

This is peerDependency revert fix version mentioned for #73

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.

We should mention in the docs users should now install @synclair/typebox by themselves.

@nadhifikbarw
Copy link
Contributor Author

@RafaelGSS i updated the readme for it

@RafaelGSS
Copy link
Member

PTAL @mcollina

@nadhifikbarw
Copy link
Contributor Author

nadhifikbarw commented Apr 29, 2023

@RafaelGSS would you mind to help check my approach for @mcollina testing concerns mentioned in #63 , I tried extending the testing workflow to perform cross peer dependency versions testing that would ease the need to specify wider version of Typebox for example ^0.26.0

https://github.com/nadhifikbarw/fastify-type-provider-typebox/tree/next

The downside is having test job numbers increase exponentially the wider the test range is for obv reason, but otherwise it's simplest approach i could think of.

Example tests run
https://github.com/nadhifikbarw/fastify-type-provider-typebox/actions/runs/4840171243

If this is okay i can also merge to PR branch

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.

lgtm.

The question on the CI comes if we want to support multiple versions of typebox outside of the current range. In that case, we need to run the tests for each one of those.

package.json Outdated Show resolved Hide resolved
@nadhifikbarw
Copy link
Contributor Author

nadhifikbarw commented Apr 30, 2023

I will keep CI changes to perform multi version test in my fork next branch, if we do ended up want to support it, I can make another PR ( ping is welcomed )

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.

lgtm

@mcollina mcollina merged commit 7206667 into fastify:main May 2, 2023
@mcollina
Copy link
Member

mcollina commented May 2, 2023

Could you send that follow-up PR?

@nadhifikbarw
Copy link
Contributor Author

@mcollina #80

@rvitaliy
Copy link

@nadhifikbarw @mcollina probably we should revert something more, since @sinclair/typebox is a peerDependency instead of dependency (again)
see changes done in this PR https://github.com/fastify/fastify-type-provider-typebox/pull/64/files

@nadhifikbarw
Copy link
Contributor Author

@rvitaliy reversion work to peer is light, but we keep oscillating on this back and forth, might be great to have final decision from maintainers on this first before doing it.

@mcollina
Copy link
Member

That is correct. All package managers now install peerDeps in a way that enable that.

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.

4 participants