Add support for class based plugins #1673

SerayaEryn · 2019-05-24T17:32:44Z

I started looking into a possible implementation of class based plugins (#1655).

There are decorators for each http method: @Get, @Post, @Put, @Delete, @Put, @Head, @Patch and @Options. In addition to that there is a @All decorator that adds the handler for all http methods. They accept the parameters like the methods on the fastify instance: @Method(path, [options])
Furthermore there is a @Hook(hook) decorator that allows to add hooks.

These decorators for the http methods and the hook decorator seem to work well.

I also have build decorators, that allow to decorate the fastify instance, request and reply.
However I don't know whether this solution is nice.
It is not possible to check whether the fastify instance, request or reply is already decorated.
And since a function can't access the this from the plugin and the request (or reply) the decorated method needs to return another function.

class MyPlugin {
  @DecorateRequest('utility')
  addUtility () {
    return function () {
      // something very useful
    }
  }
}

Maybe the following approach might be better:

class MyPlugin {
  @SupplyWithInstance // or something similar
  decorate (instance) {
    if(!instance.hasRequestDecorator('utility')) {
      instance.decorateRequest('utility', function () {
        // something very useful
      })
    }
  }
}

The documentation for the class based plugins is not finished yet.

I think this feature should be marked as experimental as long as the typescript feature is experimental and the proposal is not finalized. And maybe there should be an option that needs to be enabled?

I also noticed that it might be nice to add some kind of @Plugin(metadata) decorator to fastify-plugin, that allows to add metadata for a plugin.

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 Code of conduct

mcollina

This looks amazing, good work! I've added a few notes.

mcollina · 2019-05-27T10:28:34Z

lib/pluginDecorators.js

+'use strict'
+
+const { kPluginMetadata } = require('./symbols')
+require('reflect-metadata')


I'm -1 on adding any dependency that modifies global state. Can we do this without this library or with a work-around?

We could store it at the plugin using a symbol.

mcollina · 2019-05-27T10:29:14Z

test/classBasedPlugins/tsconfig.json

+    "target": "es6",
+    "module": "commonjs",
+    "strict": false,
+    "experimentalDecorators": true


I think I've heard they are going to change this in the next version of TS. When will decorators be out of experimental? Have you got any info?

No, I got no information on this.
However I think we will know in two days: microsoft/TypeScript#30555

Decorators are still experimental in typescript 3.5.x.

docs/ClassBasedPlugins.md

mcollina · 2019-05-27T10:33:35Z

docs/ClassBasedPlugins.md

+import { DecorateInstance } from 'fastify'
+
+class MyPlugin {
+  @DecorateInstance()


We would need to add a decorator on the class itself to enable/disable encapsulation, i.e. setting https://github.com/fastify/fastify-plugin/blob/966248ce75eebe58727aaa5c676d410b4641d236/index.js#L16

I agree. Shall we add this decorator in fastify-plugin? It could provide the other features of fastify-plugin as a decorator, too.

I opened a PR (fastify/fastify-plugin#73) in the fastify-plugin repository that adds such a decorator.

Ethan-Arrowood · 2019-05-28T17:31:09Z

Are we planning on landing this in v2?

mcollina · 2019-05-28T17:40:01Z

I would prefer typescript decorators to exit experimental status first. Likely v3. Il giorno mar 28 mag 2019 alle 19:31 Ethan Arrowood < notifications@github.com> ha scritto:

…

Are we planning on landing this in v2? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1673?email_source=notifications&email_token=AAAMXY7JO2C3BI3Z663YCIDPXVT55A5CNFSM4HPRMXU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWM3RBA#issuecomment-496613508>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMXY7MZHGIM4GI6DTTOELPXVT55ANCNFSM4HPRMXUQ> .

package.json

Ethan-Arrowood · 2019-05-28T17:50:18Z

Per @mcollina confirmation, please rebase onto next branch, and similarly, change the PR base is to next instead of master. I recently did this for the typescript refactor PR so let me know if you have issues/questions 😁👍

SerayaEryn · 2019-05-28T20:00:03Z

Ok, I'll rebase the branch and change the PR.

mcollina · 2019-06-09T16:39:51Z

lib/abstractPlugin.js

+
+  [kSetInstance] (instance) {
+    this.instance = instance
+  }


I don’t think this class is needed at all. We can just set the instance manually on the object, without using an accessor.

mcollina · 2019-06-17T14:39:27Z

test/classBasedPlugins/class-based-plugin.test.ts

    @Get('/')
    async handler (request, response) {
-      t.ok(this.instance)
+      t.ok(this['instance'])


why this change?

That wasn't planed to stay. I fixed it.

mcollina · 2019-06-17T14:40:24Z

fastify.js

+module.exports.Hook = Hook
+module.exports.DecorateRequest = DecorateRequest
+module.exports.DecorateReply = DecorateReply
+module.exports.DecorateInstance = DecorateInstance


Would it be possible to use these with JavaScript as well?

The decorators can be used with babel.
Using them without typescript or babel should be possible, but it is not very practical.

mcollina · 2019-06-17T14:44:33Z

I really like this proposal. I'm a bit scared about how this would affect our LTS support, considering that TS decorators are still experimental.

Should we land them as experimental here, maybe logging a warning?

SerayaEryn · 2019-06-17T19:31:37Z

I agree that they should be experimental and a warnung is a good idea.
I'll add a warning.

Ethan-Arrowood · 2019-06-18T16:50:07Z

If this is to land in next I'd like it to be experimental until decorators are standard.

Additionally, when #1569 lands I believe some updates will need to be made with this proposal.

stale · 2019-07-03T16:56:01Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

SerayaEryn · 2019-07-04T16:14:39Z

Just a note: I am waiting until #1569 is being merged before I add typings for the new Decorators that this PR adds.

stale · 2019-07-19T16:56:51Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

SerayaEryn · 2019-08-29T19:08:13Z

I rebased the branch since #1569 has been merged and added typings for the decorators.

SerayaEryn · 2019-08-30T06:24:39Z

The test failures seem to be related to typescript@3.6.2. With typescript@3.5.3 all tests are passing.

RafaelGSS · 2019-08-30T11:56:36Z

Hi @SerayaEryn! Really excellent addition.
But I have some doubts, based on this example retired of doc:

class MyPlugin {
  constructor (options) {
    // ...
  }
  @Get('/')
  async route (request, reply) {
    return 'hello world'
  }
}

How I can add a preHandler for each route?
The same as occurs after I do Add hook onResponse by route #1828

SerayaEryn · 2019-09-01T08:58:14Z

The decorators for the http methods have a second parameter that allows to add additional route options:

class MyPlugin {
  constructor (options) {
    // ...
  }
  @Get('/', { preHandler: yourPreHandler })
  async route (request, reply) {
    return 'hello world'
  }
}

mcollina · 2019-09-01T09:40:49Z

This work is impressive.

CI is failing, would you mind taking a look?

SerayaEryn · 2019-09-01T11:15:32Z

The new typescript version 3.6.2 seems to cause the failing tests. I got the same failing tests with that version on my local machine. With the older 3.5.3 version they pass.

I take a closer look next week.

StarpTech · 2019-09-01T12:21:34Z

Great PR but I don't like that code is shipped to the core and only typescript is supported.

StarpTech · 2019-09-01T12:29:03Z

Can we provide them as a plugin?

SerayaEryn · 2019-09-18T18:45:40Z

Can we provide them as a plugin?

I think that should be possible.

mcollina · 2019-09-20T09:28:03Z

If we can pack them as a plugin, it would be fantastic. Il giorno mer 18 set 2019 alle ore 20:45 Denis Fäcke < notifications@github.com> ha scritto:

…

Can we provide them as a plugin? I think that should be possible. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1673?email_source=notifications&email_token=AAAMXY5B5UEYR5KR7TPO3C3QKJZNLA5CNFSM4HPRMXU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7BB7CI#issuecomment-532815753>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMXY2C6WFY7XUQA3EEUOTQKJZNLANCNFSM4HPRMXUQ> .

SerayaEryn · 2019-09-28T14:17:19Z

I'll close this PR and will start working on a module as soon as i can.

github-actions · 2022-02-10T09:11:51Z

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.

mcollina requested changes May 27, 2019

View reviewed changes

Ethan-Arrowood reviewed May 28, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

SerayaEryn force-pushed the add-support-for-class-based-plugins branch from 5546fc7 to 3c13d7b Compare May 31, 2019 18:32

SerayaEryn changed the base branch from master to next May 31, 2019 18:33

mcollina reviewed Jun 9, 2019

View reviewed changes

mcollina reviewed Jun 17, 2019

View reviewed changes

SerayaEryn mentioned this pull request Jun 17, 2019

Add @Plugin(options) decorator fastify/fastify-plugin#73

Closed

stale bot added the stale Issue or pr with more than 15 days of inactivity. label Jul 3, 2019

stale bot removed the stale Issue or pr with more than 15 days of inactivity. label Jul 4, 2019

stale bot added the stale Issue or pr with more than 15 days of inactivity. label Jul 19, 2019

stale bot closed this Jul 26, 2019

jsumners added discussion Issues or PRs with this label will never stale and removed stale Issue or pr with more than 15 days of inactivity. labels Jul 26, 2019

jsumners reopened this Jul 26, 2019

SerayaEryn added 6 commits August 29, 2019 18:58

add support for class based plugins

fbf8f90

decoration of reply, request and instance

04d5520

documentation

3322674

fix coverage & coveralls task

132a585

use symbol instead of reflect-metadata

68054ad

expose fastify instance

a9861d5

SerayaEryn added 6 commits August 29, 2019 19:01

remove AbstractPlugin class

608b0c9

documentation for class based plugins

2497151

fix instance in test

d6be715

log warning on first usage

46409b1

fix rebase mistake

33d2762

add typings

9527327

SerayaEryn force-pushed the add-support-for-class-based-plugins branch from 14b637d to 9527327 Compare August 29, 2019 19:03

SerayaEryn marked this pull request as ready for review August 29, 2019 19:08

mcollina mentioned this pull request Sep 1, 2019

Support for static plugin load #1812

Closed

SerayaEryn closed this Sep 28, 2019

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

Add support for class based plugins #1673

Add support for class based plugins #1673

Conversation

SerayaEryn commented May 24, 2019

Checklist

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethan-Arrowood commented May 28, 2019

mcollina commented May 28, 2019 via email

Ethan-Arrowood commented May 28, 2019

SerayaEryn commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Jun 17, 2019

SerayaEryn commented Jun 17, 2019

Ethan-Arrowood commented Jun 18, 2019

stale bot commented Jul 3, 2019

SerayaEryn commented Jul 4, 2019

stale bot commented Jul 19, 2019

SerayaEryn commented Aug 29, 2019

SerayaEryn commented Aug 30, 2019

RafaelGSS commented Aug 30, 2019

SerayaEryn commented Sep 1, 2019

mcollina commented Sep 1, 2019

SerayaEryn commented Sep 1, 2019

StarpTech commented Sep 1, 2019

StarpTech commented Sep 1, 2019

SerayaEryn commented Sep 18, 2019

mcollina commented Sep 20, 2019 via email

SerayaEryn commented Sep 28, 2019

github-actions bot commented Feb 10, 2022