fastify/avvio

Rewrite avvio from scratch

delvedor opened this issue · 9 comments

delvedor commented

Avvio is a fantastic library and the hearth of the Fastify plugin system. In the first days of Fastify (4 years ago!!) @mcollina and I spent months working on this library, trying to figure out all the moving pieces, and I'm very happy about the outcome.
Avvio is predictable, works well with good enough performances, and has a comprehensive set of features.
Unfortunately, in the past two years, we have focused more on adding features and less on refactoring the library. I guess we can all agree it is something that should be done in the near future, as the internals has become rather complex both to understand and maintain.

This is a long term goal, but I would love to see a complete rewrite of the library by Fastify v4.
The first thing we should do, in my opinion, is to define which features are essential and which are no longer useful or too hard to maintain.
For example, register (use here) being awaitable but also returning Fastify itself is nice but complex to maintain and with its fair amount of edge cases (see #107, #114, and #116). Or .after, which was very useful in the past, but today it's just a difficult component to understand for users with its edge cases as well(see #93 and fastify/fastify-express#3).

Once we have defined which features we should keep, we should write a Fastify application that uses them all, so when we'll start the refactoring we'll make sure that all of them will still work.
We'll use this issue for tracking the effort, let's keep here the discussion about which feature should be kept and then open a separate pr for each one.
It would be nice also to ask the community how are they using the plugin system, so we are sure not to break the most used parts.

cc @fastify/core

Ethan-Arrowood commented

Recent discussions highlighted by TypeScript definitions include the handling of ESM and default exports. We should probably pay attention to CJS/ESM behavior (even from just a JS standpoint) as that landscape has seen a lot of recent changes (over the past year or so)

Edit for issue refs:

  • 👍2
jsumners commented

This is a long term goal, but I would love to see a complete rewrite of the library by Fastify v4.

This is a contradictory statement, most likely. I bet we issue a v4 much quicker than we did v3. The next Node LTS is not far off.

delvedor commented

@Ethan-Arrowood agreed! Please also add the references to the issues you are mentioning, so we have a single place where we are tracking them all.

This is a contradictory statement, most likely. I bet we issue a v4 much quicker than we did v3. The next Node LTS is not far off.

@jsumners yes, v4 will be faster, but I don't think it will happen in less than a year. My bad, "long term" is not that precise :)

  • 👍2
Eomm commented

I think:

  • drop after: since the thenable support it is a duplicated feature imho
  • define better wording per onClose: right now its logic is the same of ready, I mean a series of functions executed one by one whenever of the function stop the chain. onClose seems an event and not a push in a queue
  • fix confusing API: ready is like a shortcut per use + start because it starts the application and trigger start events - even with autostart: false
  • new notification events:
    • preStart instead of preReady
    • use: when use is called - to build a better JSON output structure of the application
mcollina commented

Note that .after() is how .then() is implemented internally.

climba03003 commented

Are we going to have any progress here?
Should we create same example usage or something that we don't want to support any longer?

Eomm commented

Should we create same example usage or something that we don't want to support any longer?

I think we should just to normalize the APIs

  • .then over .after
  • remove avvio.express()
  • remove avvio.prettyPrint()
  • add avvio.onReady (now it is implemented in fastify by tweaking avvio)
  • check the events (preReady)
  • 👍3
Uzlopak commented

I started to refactor it incrementally. Basically trying to refactor and while touching the code increase the coverage.

Should we really remove prettyPrint()? Maybe when REPL comes to fastify, it makes sense to have the opportunity to prettyPrint the status?

What about then over after? Should we just rename the after to then?

mcollina commented

Should we really remove prettyPrint()? Maybe when REPL comes to fastify, it makes sense to have the opportunity to prettyPrint the status?

I don't think so, quite a few people rely on this.

  • 👍1