Skip to content

Conversation

@cemremengu
Copy link
Contributor

This looks good to go now 👍

package.json Outdated
},
"peerDependencies": {
"fastify": ">=1.13.0"
"fastify": ">=2.0.0-rc.0"
Copy link
Member

Choose a reason for hiding this comment

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

I think this might cause trouble down the line. We should remove the peerDependency completely, it's not really needed as it's checked by fastify-plugin

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.671% when pulling 36c3a19 on fastify-next into 92708bc on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.671% when pulling 36c3a19 on fastify-next into 92708bc on master.

@coveralls
Copy link

coveralls commented Nov 27, 2018

Coverage Status

Coverage remained the same at 93.671% when pulling 0cfe37a on fastify-next into 92708bc on master.

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 with a nit

@cemremengu
Copy link
Contributor Author

@mcollina How do we progress with these ones? Bump minor/major and release now or wait for the v2 stable?

@mcollina
Copy link
Member

just do a major release, but tag it as ‘next’. We’ll retag once fastify is out of rc.

@cemremengu cemremengu merged commit 6f0b363 into master Nov 28, 2018
@cemremengu cemremengu deleted the fastify-next branch November 28, 2018 01:14
@allevo
Copy link
Member

allevo commented Nov 28, 2018

Wow you are too fast!
Sorry I'm very late!

It'ok anyway, well done! (and thanks)

@cemremengu
Copy link
Contributor Author

@allevo no problem, didn't require many changes after the previous PR actually so I just thought I should land it 😄 Thanks for the support!

Now that I think maybe this could also work with something around Fastify >=1.11.0. Should we try to lower the plugin constraint as much as possible?

@mcollina
Copy link
Member

Yes I think so.

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.

5 participants