Add toString method #861

Ethan-Arrowood · 2018-03-24T07:58:55Z

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

Closes: #703

sandbox.js will be removed when this feature is finished. It is a useful file for testing during development. I think I have to move toString.js into lib as well.

Some things to note:

This is a very simple implementation of the full toString method
I'd like to print the Plugins and Middleware for each route in a similar format to fastify.printRoutes but I'm not sure where to start with that as that method is coming from find-my-way. (Should I try to copy that function and edit it to fit our needs?)
How should the json output be structured?

delvedor

Great start!
The prettyPrint api of find-my-way is complex and needs to be refactored, you can try with https://github.com/substack/node-archy, which seems to be right for us :)

Ethan-Arrowood · 2018-03-24T10:54:07Z

Oh great! Thanks for the tip. How do access the routes on a Fastify instance? Do I need to access it through find-my-way?

delvedor · 2018-03-24T11:00:52Z

find-my-way does not exposes the registered path, @mcollina had a similar problem in fastify/fastify-routes-stats.
At the moment the only way to access the declared route url is by using the onRoute hook.
But since this is a feature implemented directly in core you can access the routes property of the router.

Ethan-Arrowood · 2018-03-24T11:15:02Z

Are Plugins tracked on the Fastify instance? AFAIK most plugins just decorate the instance (or the request/reply parts of the instance) and that is it. I wonder whats the best way to show that back to the user. -- Could this require editing .decorate and make it start storing the names or references to the new functions?

mcollina · 2018-03-24T23:36:46Z

@Ethan-Arrowood yes I think so. However we are loosing all these metadata about fastify after boot finishes. However, I would add all these stuff in steps, and this is a good one.

You might want to add some events/or metadata info to https://github.com/mcollina/avvio as plugins are tracked there. (The code in avvio is very complex).

mcollina · 2018-03-24T23:37:34Z

toString.js

+  })
+
+  console.log(`\nPlugins`)
+  console.log(`${LINES.r} TODO`)


You should not use console.log, but rather concatenate the string. In this way you can easily add a unit test.

mcollina · 2018-03-24T23:38:11Z

toString.js

+  console.log(`\nRoutes`)
+  console.log(`${this.printRoutes()}`)
+
+  console.log(`\nMiddleware`)


This should be "top level middlewares". I don't think you can see middlewares that are wrapped within a plugin.

mcollina · 2018-03-24T23:38:42Z

toString.js

+function toString() {
+  console.log('Fastify Instance toString')
+  /* Print hooks */
+  console.log(`\nHooks`)


This should be "Top level Hooks". I don't think you can see hooks that are wrapped within a plugin.

Ethan-Arrowood · 2018-04-16T22:48:14Z

Hello! I just got back from a 2 week trip and I'll be continuing work on this feature soon.

Ethan-Arrowood · 2018-04-29T23:16:30Z

@mcollina can you provide some insight into adding events/or metadata info to avvio?

mcollina · 2018-04-30T07:36:57Z

I would emit an event when a plugin start loading, and when a plugin finishes loading. However, there will be a to ton (every route is wrapped in a plugin) so some filtering might be needed.

Ethan-Arrowood · 2018-05-01T08:04:57Z

You were not kidding when you said this code is complex!

I found this line in fastify:

fastify/fastify.js

Line 397 in 953616f

    
           instance[pluginUtils.registeredPlugins] = Object.create(instance[pluginUtils.registeredPlugins])

However, it doesn't seem to be tracking plugins. Do you think I should try pumping plugin metadata into there?

I've also begun cracking away at avvio. I've added an event emitter after a plugin is loaded. This does not emit for routes or hooks or middleware.

Boot.prototype.use = function (plugin, opts) {
  if (typeof plugin === 'function') {
    console.log(`New plugin (${plugin.name}) loading . . .`)
    this._addPlugin(plugin, opts, false)
    this.emit(`Loaded plugin (${plugin.name})`)
  } else {
    throw new Error('plugin must be a function')
  }

  return this
}

I guess the next step is to listen for this event in the fastify object and then pipe the name of the function into the registeredPlugins list.

mcollina

Sorry for the long delay for a review, I’ve been on vacation and then catching up for a while (and traveling for work).

mcollina · 2018-05-06T15:24:35Z

fastify.js


+>>>>>>> 43d9c92... prep for pull


is this expected?

mcollina · 2018-05-06T15:25:13Z

fastify.js

@@ -23,6 +24,7 @@ const Schemas = require('./lib/schemas')
 const loggerUtils = require('./lib/logger')
 const pluginUtils = require('./lib/pluginUtils')
 const runHooks = require('./lib/hookRunner').hookRunner
+const toString = require('./toString')


this should live under lib/

mcollina · 2018-05-06T15:26:29Z

toString.js

+  output_string += `\n${LINES.r} TODO`
+  console.log(`Registered Plugins: ${this[pluginUtils.registeredPlugins]}`)
+  // console.log(this)
+  console.log(output_string)


no console.log in this method

mcollina · 2018-05-06T15:27:36Z

This needs unit tests.

Ethan-Arrowood · 2018-05-07T08:21:08Z

Not to worry about the delay. I've been quite busy myself. Thank you for the review. I'll make sure to fix these things soon, but can you please review my previous comment on this thread regarding pluginUtils.registeredPlugins? Once that is figure out I can start polishing this up. Thank you!

mcollina · 2018-05-07T11:12:40Z

The registeredPlugins only track names. Basically that array tells which plugins have been loaded in the current tree, but it does not represent the tree itself at the root. See

fastify/fastify.js

Line 397 in 953616f

    
           instance[pluginUtils.registeredPlugins] = Object.create(instance[pluginUtils.registeredPlugins])

.

I think you can extend that mechanism to track down the tree of loaded plugins, without touching avvio at all.

Ethan-Arrowood · 2018-05-07T11:42:33Z

Okay interesting. I'll mess around with that a bit more and see what I can make of it.

Ethan-Arrowood · 2018-06-10T05:38:21Z

(branch has been rebased properly)
@mcollina / @delvedor I noticed that fastify was never actually tracking plugins. I've updated the .register method to now do so but had to use a weird work around with avvio.

The way it works is the fastify instance inherits the .use method from avvio and assigns it to fastify.register. Then it declares its own .use method (for registering middleware). But in order to track plugins being added I needed to expand the fastify.register command to push the added plugin to fastify[pluginUtils.registeredPlugins]. Let me know if you see a better way to do this.

Also I didn't know what we should do about anonymously named plugins so I added two things:

A warning that users should name their plugins
The toString method concatenates the string 'anonymous' with the index of the plugin and adds that to the output string.

You can check out this behavior by running the included sandbox.js file.

If you think the toString method is at a good place I can work on unit tests next.

mcollina

tests are also failing

mcollina · 2018-06-10T11:36:04Z

fastify.js

+    fastify.avvioUse(plugin, options)
+    if (plugin.name.length === 0) {
+      log.warn('Warning: should not use anonymous plugins. Provide a function name on declaration.')
+    }


I don’t think emitting a warn is feasible. We should have default strategy (maybe the filename? or the closest name in package.json) in case there is no name.

How would this method have access to the filename? Even if the user uses:

// plugin.js const fp = require('fastify-plugin') module.exports = fp((instance, opts, next) => { instance.decorate('mult', (a, b) => a * b) next() })

// app.js fastify.register(require('plugin.js'))

The register method will still receive an anonymous function.

Right now I am using 'anonymous'+plugin_index

mcollina · 2018-06-10T11:36:23Z

fastify.js

@@ -173,7 +173,14 @@ function build (options) {
  fastify.setSchemaCompiler(buildSchemaCompiler())

  // plugin
-  fastify.register = fastify.use
+  fastify.avvioUse = fastify.use


there is no need to expose this, store it in a variable.

Ethan-Arrowood · 2018-06-10T13:11:05Z

I'm not able to get the linter to successfully run. Not sure what I broke or what I'm doing wrong but this is my console output:

It is the same output when I run npm run lint or npm run test

Any idea whats going on here?

I've deleted node_modules
Ran npm install
I'm using node 10.4.0 and npm 6.1.0

Made this into a proper issue (#968)

mcollina · 2018-07-01T21:09:28Z

@Ethan-Arrowood fastify-plugin 1.2.0 is out.

nwoltman · 2018-07-02T04:46:21Z

docs/Plugins.md

@@ -79,37 +79,27 @@ Sometimes, you will need to know when the server is about to close, for example

 Do not forget that `register` will always create a new Fastify scope, if you don't need that, read the following section.

-If you register an anonymous function using `fastify-plugin`, it will automatically gain a new property based on the file name the `.fp()` method was called from. You can access it using `Symbol.for('fastify.display-name')`. See the example below.
+If you register an anonymous function using `fastify-plugin`, it will automatically set the meta option `name` to the file name the `.fp()` method was called from. You can access it using `fn[Symbol.for('plugin-meta')].name` or `fn[Symbol.for('fastify.display-name')]`. See the example below. If you set the name using the meta property, the file name will be ignored and the `fastify.display-name` will be set to the meta name as well.


All references to fastify.display-name should be removed since it will be removed in the next major version of fastify-plugin.

Also, .fp() should just be fp().

nwoltman · 2018-07-02T04:48:22Z

docs/Plugins.md

-  name: 'my-plugin'
-}))
-```
+It is recommended you use a named function, but anonymous functions are supported. This was added for use within `fastify.toString()`. 


IMO the recommended method of setting a plugin's name should be with the meta name option.

mcollina · 2018-07-02T07:59:28Z

@nwoltman if we go with that plan, then we need to document plugin-meta somewhere. fastify-plugin is a mere utility and it should be possible to do the same things without it.

Ethan-Arrowood · 2018-07-02T11:29:48Z

Let me see what I can come up with for the documentation. Basic facts:

When using fastify-plugin it is recommended to use the meta.name property, then to use a function name, and finally an anonymous function is also supported (will auto-use file name).
Without fastify-plugin it is recommended to use a named function; otherwise it will use a 'anonymous' + index instead.

Here is an example:

// toString.test.js
fastify.register(fp((instance, opts, next) => {
  instance.decorate('anonymousPlugin', (a, b) => a + b)
  next()
}))

fastify.register(fp(function standardPlugin (instance, opts, next) {
  instance.decorate('standardPlugin', (a, b) => a - b)
  next()
}))

fastify.register((instance, opts, next) => {
  instance.decorate('anonWithoutFastifyPlugin', (a, b) => a * b)
  next()
})

fastify.register(function namedWithoutFP (instance, opts, next) {
  instance.decorate('namedWithoutFP', (a, b) => a / b)
  next()
})

// fastify.toString() ->
let result = `...
Plugins
├── toString.test
├── standardPlugin
├── anonymous0
└── namedWithoutFP`

nwoltman · 2018-07-02T15:17:13Z

@mcollina You're right. Using fastify-plugin shouldn't be a requirement to naming a plugin.

nwoltman

LGTM with a nit

nwoltman · 2018-07-02T15:19:32Z

docs/Plugins.md

+fastify.register(fp(async () => {}, { name: 'myAsyncFunction' }))
+```
+
+Internally, `fastify` will register this anonymous function as `'myAsyncFunction'`. `fastify-plugin` will always prioritize the meta property, even when supplied a named function. If you do not provide a `meta.name` to the `fp()` method, `fastify-plugin` will try and use the function name; if its an anonymous function it will use the file name from which the `fp()` method was called. See the examples below:


nit: if its => if it's

mcollina

Still LGTM

delvedor

Hello! Nice work!
I have some questions, given the following example

'use strict'

const fastify = require('fastify')()

fastify.get('/', () => {})

fastify.get('/hello', () => {})

fastify.register((instance, opts, next) => {
  instance.addHook('preHandler', () => {})
  instance.get('/hello', () => {})

  instance.register((i, opts, next) => {
    i.addHook('preHandler', () => {})
    i.get('/hello', () => {})

    next()
  }, { prefix: '/inside' })

  next()
}, { prefix: '/v1' })

fastify.ready(() => {
  console.log(fastify.toString())
})

I get

Top Level Hooks
├── onRequest (0 hooks): []
├── preHandler (0 hooks): []
├── onResponse (0 hooks): []
└── onSend (0 hooks): []

Routes
└── / (GET)
    ├── hello (GET)
    └── v1/
        ├── hello (GET)
        └── inside/ciao (GET)

Top Level Middleware
└── /: []

Plugins
└── anonymous0

As you can see the nested hooks (and probably the same for middlewares) are not present in the final string.
Why? Am I missing something?

If this is correct, I think we should not land this until we have a complete log of every level of Fastify, because we are not giving a complete overview of the application structure, but just a small part.

Ethan-Arrowood · 2018-07-03T13:00:08Z

Nested hooks were intentionally left out as of right now. Looking back on your original issue I see how we may want to display this information.

So per your example the expected out put might look like something like this?

Hooks: []
├── /
│
├── hello (GET)
│
└── v1/ 
    │   Hooks: [preHandler]
    │
    ├── hello (GET) 
    │
    └── inside/hello (GET)
        └── Hooks: [preHandler]

If this is what you want it to be instead of what it currently is then this feature will need quite a bit more work (which I'm okay with doing). I do not know how Fastify exposes these things internally so I cannot gauge how difficult this change will be.

delvedor · 2018-07-03T13:11:27Z

@Ethan-Arrowood yes, kind of:

Hooks: []
Middlewares: []
├── /
├── hello (GET)
│
└── v1/ 
│   │   Hooks: [preHandler]
│   │   Middlewares: []
│   ├── hello (GET) 
│   ├── status/check (HEAD) 
│   │
│   └── inside/
│       │   Hooks: [preHandler]
│       │   Middlewares: []
│       ├── hello (GET) 
│       └── hello/update (POST)
│
└── v2/ 
    │   Hooks: [preHandler, onSend]
    │   Middlewares: []
    ├── hello (GET) 
    └── status/check (HEAD)

I think we can drop the use of printRoutes and build our own method. We might want to use node-archy to build the tree.
It's definitely not an easy task, but we will give you support everywhere we can!

Probably a nice solution could be add an internal event emitter which emits an event for every route/hook/middleware/plugin registered with its own context. If you want I can work on it :)

Ethan-Arrowood · 2018-07-03T15:03:50Z

Thank you for the recommendations. I think I'll take on the challenge and see how far I can go on my own. I'll make sure to ping here if i need help!

allevo · 2018-07-11T15:34:09Z

lib/pluginUtils.js

-  this[registeredPlugins].push(name)
+  const functionName = meta && meta.name ? meta.name : (
+    !fn.name ? `anonymous${anonFuncCount++}` : (
+      fn.name !== 'bound _after' ? fn.name : null


What is the 'bound _after' ? Could you use a constant for that?

This is some default name for one of things fastify registers or uses. Will most likely change when I re implement this feature given the new constraints

stale · 2018-07-26T21:24:47Z

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.

Ethan-Arrowood · 2018-07-29T23:35:50Z

Sorry I haven't gotten back around to this yet. Been very busy with my new internship so i'm only just starting to find time for open source

jsumners · 2018-07-30T00:24:45Z

Completely understand. I'm right there with you.

jsumners · 2018-10-09T23:07:46Z

I vote yes for toJSON.

mcollina · 2018-10-10T01:08:52Z

+1 Il giorno mar 9 ott 2018 alle 16:07 James Sumners <notifications@github.com> ha scritto:

…

I vote yes for toJSON. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#861 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL42tf3ZuUwIXhIQuJ896Y3o0ieTRlks5ujSxCgaJpZM4S5pUW> .

Ethan-Arrowood · 2019-01-11T23:43:21Z

I don't believe I'll be able to complete this feature. There is an toJSON pr open here: #1261
if/when that is complete I'll be happy to work on the toString feature (which will use that toJSON object as its base.

cemremengu · 2019-01-11T23:45:56Z

@Ethan-Arrowood you/we can work on the toJSON if you have time. I opened the PR but failed to deliver due to my schedule.

Ethan-Arrowood · 2019-01-11T23:57:31Z

@cemremengu i'll look into it but I ran into a fair amount of discouraging issue trying to implement it before. Maybe after this time I'll have some new ideas, so we'll see, but I don't want to make any promises

github-actions · 2022-02-11T00:14:43Z

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.

delvedor reviewed Mar 24, 2018

View reviewed changes

delvedor added feature semver-minor Issue or PR that should land as semver minor labels Mar 24, 2018

mcollina reviewed Mar 24, 2018

View reviewed changes

Ethan-Arrowood mentioned this pull request May 1, 2018

Plugin event emitter fastify/avvio#63

Closed

Ethan-Arrowood force-pushed the add-toString branch from 26ab4fc to e418234 Compare May 2, 2018 00:38

mcollina reviewed May 6, 2018

View reviewed changes

Ethan-Arrowood added 4 commits June 10, 2018 15:51

simple toString implementation

dba06be

simple toString implementation

5a35b13

Convert toString method to concat instead of console log

b2dee56

add plugin tracking functionality to fastify and toString

eb95b10

Ethan-Arrowood force-pushed the add-toString branch from 2cf83cb to eb95b10 Compare June 10, 2018 05:52

mcollina reviewed Jun 10, 2018

View reviewed changes

Ethan-Arrowood added 3 commits June 10, 2018 23:16

Remove testing files and move toString to lib

80be3c5

Original unit tests now pass. Removed log from toString

4ca0fb9

add basic unittest

af4492e

Update to fp 1.2.0

405299f

nwoltman reviewed Jul 2, 2018

View reviewed changes

Update documentation and support non fastify-plugin functions

6caf7ee

nwoltman approved these changes Jul 2, 2018

View reviewed changes

correct spelling

33c2ecc

mcollina approved these changes Jul 3, 2018

View reviewed changes

delvedor reviewed Jul 3, 2018

View reviewed changes

allevo reviewed Jul 11, 2018

View reviewed changes

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

mcollina added the discussion Issues or PRs with this label will never stale label Jul 26, 2018

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

Ethan-Arrowood mentioned this pull request Oct 4, 2018

Feature: toString method #703

Closed

Ethan-Arrowood closed this Jan 11, 2019

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

Add toString method #861

Add toString method #861

Conversation

Ethan-Arrowood commented Mar 24, 2018 • edited

Checklist

delvedor left a comment

Choose a reason for hiding this comment

Ethan-Arrowood commented Mar 24, 2018

delvedor commented Mar 24, 2018

Ethan-Arrowood commented Mar 24, 2018

mcollina commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethan-Arrowood commented Apr 16, 2018

Ethan-Arrowood commented Apr 29, 2018

mcollina commented Apr 30, 2018

Ethan-Arrowood commented May 1, 2018 • edited

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

mcollina commented May 6, 2018

Ethan-Arrowood commented May 7, 2018

mcollina commented May 7, 2018

Ethan-Arrowood commented May 7, 2018

Ethan-Arrowood commented Jun 10, 2018 • edited

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethan-Arrowood Jun 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethan-Arrowood commented Jun 10, 2018 • edited

mcollina commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Jul 2, 2018

Ethan-Arrowood commented Jul 2, 2018

nwoltman commented Jul 2, 2018

nwoltman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

delvedor left a comment

Choose a reason for hiding this comment

Ethan-Arrowood commented Jul 3, 2018 • edited

delvedor commented Jul 3, 2018 • edited

Ethan-Arrowood commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jul 26, 2018

Ethan-Arrowood commented Jul 29, 2018

jsumners commented Jul 30, 2018

jsumners commented Oct 9, 2018

mcollina commented Oct 10, 2018 via email

Ethan-Arrowood commented Jan 11, 2019

cemremengu commented Jan 11, 2019

Ethan-Arrowood commented Jan 11, 2019

github-actions bot commented Feb 11, 2022

Ethan-Arrowood commented Mar 24, 2018 •

edited

Ethan-Arrowood commented May 1, 2018 •

edited

Ethan-Arrowood commented Jun 10, 2018 •

edited

Ethan-Arrowood Jun 10, 2018 •

edited

Ethan-Arrowood commented Jun 10, 2018 •

edited

Ethan-Arrowood commented Jul 3, 2018 •

edited

delvedor commented Jul 3, 2018 •

edited