-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: Add plugin version check #2549
Conversation
lib/pluginOverride.js
Outdated
@@ -48,6 +53,7 @@ module.exports = function override (old, fn, opts) { | |||
instance.getSchemas = instance[kSchemas].getSchemas.bind(instance[kSchemas]) | |||
instance[pluginUtils.registeredPlugins] = Object.create(instance[pluginUtils.registeredPlugins]) | |||
instance[kPluginNameChain] = [pluginUtils.getPluginName(fn) || pluginUtils.getFuncPreview(fn)] | |||
instance[kVersion] = version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make this private? It might be useful for others if fastifyInstance.version
returned the correct value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, you want to mutate old
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not need this stored per-instance. You can store it as a top level value.
lib/pluginUtils.js
Outdated
const requiredVersion = meta.fastify | ||
if (!requiredVersion) return | ||
|
||
assert(semver.lte(this[kVersion], requiredVersion), `fastify-plugin: ${meta.name} - expected '${requiredVersion}' fastify version, '${this[kVersion]}' is installed`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind to use an error with code (see lib/errors file).
lib/pluginOverride.js
Outdated
|
||
function loadVersion () { | ||
const pkg = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'package.json'))) | ||
return pkg.version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be cached.
As a suggestion I would put it in a getter on the top level fastify function (the factory).
lib/pluginOverride.js
Outdated
@@ -48,6 +53,7 @@ module.exports = function override (old, fn, opts) { | |||
instance.getSchemas = instance[kSchemas].getSchemas.bind(instance[kSchemas]) | |||
instance[pluginUtils.registeredPlugins] = Object.create(instance[pluginUtils.registeredPlugins]) | |||
instance[kPluginNameChain] = [pluginUtils.getPluginName(fn) || pluginUtils.getFuncPreview(fn)] | |||
instance[kVersion] = version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not need this stored per-instance. You can store it as a top level value.
@@ -581,6 +590,11 @@ function wrapRouting (httpHandler, { rewriteUrl, logger }) { | |||
} | |||
} | |||
|
|||
function loadVersion () { | |||
const pkg = JSON.parse(fs.readFileSync(path.join(__dirname, 'package.json'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why it makes more sense to read and parse manually rather than require it? Do we want to avoid caching it for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. To avoid keeping the "large" object in memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require involves caching the required file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs some docs
fastify.js
Outdated
get () { return this[kSerializerCompiler] } | ||
}, | ||
version: { | ||
get () { return this[kVersion] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would load the version number dynamically on-demand and then cache it at the top of the file, similar to lightMyRequest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we use fs
and path
only for this, it might be interesting to only require them in the loadVersion
method no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we use
fs
andpath
only for this, it might be interesting to only require them in theloadVersion
method no?
I think so, we did it per light-my-request
lib/pluginUtils.js
Outdated
const requiredVersion = meta.fastify | ||
if (!requiredVersion) return | ||
|
||
if (semver.lte(this[kVersion], requiredVersion)) throw new FST_ERR_PLUGIN_VERSION_MISMATCH(meta.name, requiredVersion, this[kVersion]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (semver.lte(this[kVersion], requiredVersion)) throw new FST_ERR_PLUGIN_VERSION_MISMATCH(meta.name, requiredVersion, this[kVersion]) | |
if (semver.lte(this.version, requiredVersion)) throw new FST_ERR_PLUGIN_VERSION_MISMATCH(meta.name, requiredVersion, this[kVersion]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
lib/pluginUtils.js
Outdated
const requiredVersion = meta.fastify | ||
if (!requiredVersion) return | ||
|
||
if (semver.lte(this.version, requiredVersion)) throw new FST_ERR_PLUGIN_VERSION_MISMATCH(meta.name, requiredVersion, this.version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our plugins we have patterns like '>=3.x'
In this case semver.lte('3.3.1', '>=3.x')
it will throw or Am I missing something?
https://github.com/fastify/fastify-oauth2/blob/master/index.js#L144
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed will have to use validerange of semver. Will fix and add tests for this. Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have some canary project to test this feature across plugins?
cc @fastify/plugins
we don't - we should rely on manually checking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Could we arrange a repo for that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! nice work
May I get a matching PR to fastify-plugin? |
Will do. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Following #2507
Added the check of version in pluginOverride
Checklist
npm run test
andnpm run benchmark
and the Code of conduct