Skip to content
This repository has been archived by the owner on Jul 8, 2023. It is now read-only.

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

Open
ivansieder opened this issue Jun 9, 2021 · 16 comments
Open

Comments

@ivansieder
Copy link
Contributor

ivansieder commented Jun 9, 2021

馃殌 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 })
@jsumners
Copy link
Member

jsumners commented Jun 9, 2021

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 nodejs/node#33954.

@ivansieder
Copy link
Contributor Author

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 ^^

@Eomm
Copy link
Member

Eomm commented Jun 9, 2021

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?

@ivansieder
Copy link
Contributor Author

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

@Eomm
Copy link
Member

Eomm commented Jun 9, 2021

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

@jsumners
Copy link
Member

jsumners commented Jun 9, 2021

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

@mcollina
Copy link
Member

I normally use the following everywhere:

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

@climba03003
Copy link
Member

climba03003 commented Jun 10, 2021

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) {
  //...
})

@mcollina
Copy link
Member

That is also a good choice.

@ivansieder
Copy link
Contributor Author

ivansieder commented Jun 11, 2021

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 ivansieder changed the title Unify the way of importing and initializing the app. Unify the way of importing and initializing the app and other pieces Jun 12, 2021
@delvedor
Copy link
Member

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.

@ivansieder
Copy link
Contributor Author

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

@mcollina
Copy link
Member

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

@climba03003
Copy link
Member

climba03003 commented Jun 19, 2021

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.

@mcollina
Copy link
Member

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

@luisorbaiceta
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants