feat: Add plugin version check by zekth · Pull Request #2549 · fastify/fastify

zekth · 2020-09-09T20:07:49Z

Following #2507
Added the check of version in pluginOverride

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

jsumners · 2020-09-10T12:05:35Z

lib/pluginOverride.js

  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


Do we need to make this private? It might be useful for others if fastifyInstance.version returned the correct value.

If so, you want to mutate old ?

I don't understand the question.

we do not need this stored per-instance. You can store it as a top level value.

mcollina · 2020-09-10T13:43:07Z

lib/pluginUtils.js

+  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`)


Would you mind to use an error with code (see lib/errors file).

mcollina · 2020-09-10T13:55:16Z

lib/pluginOverride.js

+
+function loadVersion () {
+  const pkg = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'package.json')))
+  return pkg.version


this should be cached.

As a suggestion I would put it in a getter on the top level fastify function (the factory).

mcollina · 2020-09-10T13:56:14Z

lib/pluginOverride.js

  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


we do not need this stored per-instance. You can store it as a top level value.

fastify.js

kibertoad · 2020-09-10T15:31:07Z

fastify.js

 }

+function loadVersion () {
+  const pkg = JSON.parse(fs.readFileSync(path.join(__dirname, 'package.json')))


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?

Yes. To avoid keeping the "large" object in memory.

require involves caching the required file

mcollina

This also needs some docs

mcollina · 2020-09-10T16:57:44Z

fastify.js

+      get () { return this[kSerializerCompiler] }
+    },
+    version: {
+      get () { return this[kVersion] }


I would load the version number dynamically on-demand and then cache it at the top of the file, similar to lightMyRequest.

Indeed good point

As we use fs and path only for this, it might be interesting to only require them in the loadVersion method no?

As we use fs and path only for this, it might be interesting to only require them in the loadVersion method no?

I think so, we did it per light-my-request

mcollina · 2020-09-10T16:58:29Z

lib/pluginUtils.js

+  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])


Suggested change

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])

mcollina

lgtm

Eomm · 2020-09-10T18:29:36Z

lib/pluginUtils.js

+  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)


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

Indeed will have to use validerange of semver. Will fix and add tests for this. Good catch

docs/Server.md

jsumners

LGTM

Eomm

Do we have some canary project to test this feature across plugins?
cc @fastify/plugins

mcollina · 2020-09-14T10:39:15Z

Do we have some canary project to test this feature across plugins?

we don't - we should rely on manually checking.

RafaelGSS

LGTM.

Eomm · 2020-09-14T17:50:24Z

Do we have some canary project to test this feature across plugins?

we don't - we should rely on manually checking.

Could we arrange a repo for that?
I never did it, so I would like to try 👍🏼

fox1t

LGTM

Ethan-Arrowood

lgtm! nice work

mcollina · 2020-09-15T06:28:11Z

May I get a matching PR to fastify-plugin?

zekth · 2020-09-15T07:17:49Z

May I get a matching PR to fastify-plugin?

Will do.

github-actions · 2022-02-07T00:13:45Z

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.

feat: Add plugin version check

7be3fe9

jsumners reviewed Sep 10, 2020

View reviewed changes

mcollina requested changes Sep 10, 2020

View reviewed changes

fix: review

0ce16ea

jsumners reviewed Sep 10, 2020

View reviewed changes

fastify.js Outdated Show resolved Hide resolved

kibertoad reviewed Sep 10, 2020

View reviewed changes

fix: review

3731792

mcollina requested changes Sep 10, 2020

View reviewed changes

zekth added 3 commits September 10, 2020 19:08

fix: review

42ff36b

fix: add typing

876e356

doc: add version

f548021

mcollina approved these changes Sep 10, 2020

View reviewed changes

Eomm requested changes Sep 10, 2020

View reviewed changes

jsumners reviewed Sep 10, 2020

View reviewed changes

docs/Server.md Outdated Show resolved Hide resolved

zekth added 2 commits September 10, 2020 21:04

fix: review

78700ef

fix: review

e8ef662

jsumners approved these changes Sep 10, 2020

View reviewed changes

Eomm approved these changes Sep 10, 2020

View reviewed changes

Eomm added semver-minor Issue or PR that should land as semver minor v3.x Issue or pr related to Fastify v3 labels Sep 10, 2020

RafaelGSS approved these changes Sep 14, 2020

View reviewed changes

fox1t approved these changes Sep 14, 2020

View reviewed changes

Ethan-Arrowood approved these changes Sep 14, 2020

View reviewed changes

mcollina merged commit 797a9db into fastify:master Sep 15, 2020

zekth deleted the plugin_version_chk branch September 15, 2020 07:17

zekth mentioned this pull request Sep 15, 2020

feat: delete version testing fastify/fastify-plugin#107

Merged

4 tasks

fralonra mentioned this pull request Sep 20, 2020

Update: f2982a2...29c2af1 fastify/docs-chinese#232

Merged

agramian mentioned this pull request Oct 6, 2020

Plugin registration no longer works with webpack #2603

Closed

zekth mentioned this pull request Oct 7, 2020

feat: add conditional load of version #2605

Merged

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2022

	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])

Uh oh!

Conversation

zekth commented Sep 9, 2020

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 14, 2020

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

Eomm commented Sep 14, 2020

Uh oh!

fox1t left a comment

Choose a reason for hiding this comment

Uh oh!

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 15, 2020

Uh oh!

zekth commented Sep 15, 2020

Uh oh!

github-actions bot commented Feb 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants