Skip to content
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

Rewrite avvio from scratch #118

Open
delvedor opened this issue Jul 14, 2020 · 9 comments · May be fixed by #199
Open

Rewrite avvio from scratch #118

delvedor opened this issue Jul 14, 2020 · 9 comments · May be fixed by #199
Labels
discussion Issues or PRs with this label will never stale semver-major Issue or PR that should land as semver major

Comments

@delvedor
Copy link
Member

delvedor commented Jul 14, 2020

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

@delvedor delvedor added semver-major Issue or PR that should land as semver major discussion Issues or PRs with this label will never stale labels Jul 14, 2020
@delvedor delvedor pinned this issue Jul 14, 2020
@Ethan-Arrowood
Copy link
Member

Ethan-Arrowood commented Jul 14, 2020

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:

@jsumners
Copy link
Member

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
Copy link
Member Author

delvedor commented Jul 14, 2020

@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 :)

@Eomm
Copy link
Member

Eomm commented Sep 12, 2020

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
Copy link
Member

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

@climba03003
Copy link
Member

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
Copy link
Member

Eomm commented Dec 1, 2022

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)

@climba03003 climba03003 linked a pull request Jan 9, 2023 that will close this issue
11 tasks
@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 14, 2023

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issues or PRs with this label will never stale semver-major Issue or PR that should land as semver major
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants