website-metalsmith icon indicating copy to clipboard operation
website-metalsmith copied to clipboard

Unify the way of importing and initializing the app and other pieces

Open ivansieder opened this issue 3 years ago • 16 comments

🚀 Feature Proposal

I'd suggest three minor things to enhance the docs:

  • unify the way of importing and initializing the app throughout the docs so that they're the same across the whole docs
  • as discussed with Matteo, it might make sense to rename references to fastify/server to app throughout the docs.
  • for the basic examples, I'd suggest to keep all the examples with the same configuration (which is to enable the logger)
  • name name the parameters in the docs consistently (sometimes it's req and sometimes it's request)

Motivation

Currently, there are different examples on how Fastify is being imported and how the app is being initialized (see below). Also, with the rise of ESM-Modules, imports mostly shift from something like const Fastify = require('fastify') to import Fastify from 'fastify', which makes things, such as const app = require('fastify')({ logger: true }) impossible. Also, it's less of a mind-shifting to use the same terms for the same thing (using the term app instead of using fastify or server for actually the same thing)

Currently, these are the two variations: JavaScript (shortened):

const fastify = require('fastify')({
  logger: true
})

TypeScript (shortened):

import Fastify from 'fastify'
const server = Fastify({})

Example

With this proposal, the above examples and all examples throughout the documentation would be unified in terms of how to name and initialize the app. Also, the example of directly importing and initializing the app in one step will be split up into an import and a separate initialization. This avoids varying terms like fastify and server for actually the same thing.

JavaScript (shortened):

const Fastify = require('fastify')
const app = Fastify({ logger: true })

TypeScript (shortened):

import Fastify from 'fastify'
const app = Fastify({ logger: true })

ivansieder avatar Jun 09 '21 19:06 ivansieder

Also, with the rise of ESM-Modules, imports mostly shift from something like const Fastify = require('fastify') to import Fastify from 'fastify', which makes things, such as const app = require('fastify')({ logger: true }) impossible.

Only if the author decides to use ESM. ESM is not necessary and it is not promoted as the primary syntax -- see https://github.com/nodejs/node/issues/33954.

jsumners avatar Jun 09 '21 19:06 jsumners

Totally agree with that @jsumners, that's also why the above examples/suggestions still include CommonJS' require. My thoughts behind this proposal were mostly related to the fact of unifying the import and initialization.

Other than that: damn, a reply within 3 mins, that's not bad ^^

ivansieder avatar Jun 09 '21 19:06 ivansieder

I agree to unify the docs and for CJS or ESM I saw a simple switch that shows the syntax chosen by the user (I cant remember which module)

I would like to unity also the plugin instance arg:

  fastify.register(function plugin (pluginInstance, opts, next) {
    next()
  })

or appInstance / appChildInstance at this stage?

Eomm avatar Jun 09 '21 20:06 Eomm

@Eomm what about renaming them to app, too? image

ivansieder avatar Jun 09 '21 20:06 ivansieder

Personally, I think app is misleading because it seems the main root application. This is true only when the user uses the fastify-plugin and not when a new context is being created.

For this reason, I would like to think if there are better options

Eomm avatar Jun 09 '21 21:06 Eomm

I think app implies Express ways of thinking. But I don't have any strong preference as long as the documentation makes sense.

jsumners avatar Jun 09 '21 21:06 jsumners

I normally use the following everywhere:

  app.register(async function plugin (app, opts) {
   
  })

mcollina avatar Jun 10 '21 10:06 mcollina

For me, it will be all fastify. It is clear to me that the variable should be an instance of fastify.

import Fastify from 'fastify'
const fastify = Fastify({ logger: true })

fastify.register(async function (fastify, options) {
  //...
})

climba03003 avatar Jun 10 '21 10:06 climba03003

That is also a good choice.

mcollina avatar Jun 10 '21 16:06 mcollina

What plays towards app for me is that I'm quiet faster to type it than fastify and it's also less prone to get it wrong (although with editor support this shouldn't be that much of an issue anymore). Other than that: by using app or server you have the same name for the file, too. Naming a file fastify would sound somewhat a little bit strange to me, but I guess that's mostly just personal preference

ivansieder avatar Jun 11 '21 17:06 ivansieder

unify the way of importing and initializing the app throughout the docs so that they're the same across the whole docs

100% agree! Furthermore, I'll be all in to always use ESM, and it will be the future of the language, so better adopt it sooner rather than later. Even if currently it does not offer all the features of CJS. Still, we should keep a CJS example somewhere.

as discussed with Matteo, it might make sense to rename references to fastify/server to app throughout the docs.

I've always used fastify., but I'm fine with app. as well. What I like about fastify. is that it always reminds about what you are using, and if/when users will share a snippet, the fastify "brand" will always be there.

for the basic examples, I'd suggest to keep all the examples with the same configuration (which is to enable the logger)

👍

name name the parameters in the docs consistently (sometimes it's req and sometimes it's request)

Yes! Given this is a documentation there is no reason to save chars, let's go for the full name of every parameter, is more clear and welcoming with new users.

delvedor avatar Jun 15 '21 06:06 delvedor

So could I go with fastify? And if so, should the files also be called fastify.js instead of server.js in that case?

ivansieder avatar Jun 18 '21 17:06 ivansieder

The files should not be called fastify as it would be ambiguous.

mcollina avatar Jun 18 '21 18:06 mcollina

File should be either app.js, server.js or something similar. For what I am currently doing. app.js is the logic for my application without .listen. server.js is how we like to start the app.js, it can be simply .listen or using aws-lambda-fastify.

Spliting application logic and starting server is better if you need to test your application and if you have multiple way to start your application.

climba03003 avatar Jun 19 '21 01:06 climba03003

I recommend using app.js as a factory and server.js as the entry point.

mcollina avatar Jun 19 '21 07:06 mcollina

I think this should be transferred either to fastify/fastify-snippet or to fastify/fastify

luisorbaiceta avatar Dec 21 '21 11:12 luisorbaiceta