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

Improve semver check for plugins #3882

Closed
mcollina opened this issue May 5, 2022 · 6 comments
Closed

Improve semver check for plugins #3882

mcollina opened this issue May 5, 2022 · 6 comments
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor v4.x Issue or pr related to Fastify v4

Comments

@mcollina
Copy link
Member

mcollina commented May 5, 2022

Replace the semver check in

const sanitizedVersion = this.version.replace(/-rc.\d+$/, '')
with:

const fastifyRc = /-rc.+$/.test(this.version)
if (requiredVersion && semver.satisfies(santizedVersion, requiredVersion, { includePrerelease: fastifyRc }) === false) {

https://github.com/npm/node-semver/tree/c56a701f45653940ee8536eafe43b3e46c11d6cc#functions

Originally posted by @jsumners in #3879 (comment)

@mcollina mcollina added v4.x Issue or pr related to Fastify v4 enhancement good first issue Good for newcomers labels May 5, 2022
@CynoidIT
Copy link
Contributor

CynoidIT commented May 5, 2022

Would love to help out. What do I need to do to start contributing? Meaning, onboarding, testing, or anything else.

@CynoidIT
Copy link
Contributor

CynoidIT commented May 5, 2022

Was looking at the code and am not sure why "&& !semver" was changed to "&& semver" in the suggested replacement. Is that intentional or typo?

@mcollina
Copy link
Member Author

mcollina commented May 5, 2022

If all test pass it's ok!

@jsumners
Copy link
Member

jsumners commented May 5, 2022

The conditional was made explicit instead of a coercive check.

@CynoidIT
Copy link
Contributor

CynoidIT commented May 6, 2022

I see. Thanks. PR is up.

@Fdawgs
Copy link
Member

Fdawgs commented May 8, 2022

Closed by #3886

@jsumners jsumners closed this as completed May 8, 2022
@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor v4.x Issue or pr related to Fastify v4
Projects
None yet
Development

No branches or pull requests

5 participants