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

Create basic architecture #1

Open
wants to merge 107 commits into
base: main
Choose a base branch
from

Conversation

jean-michelet
Copy link

@jean-michelet jean-michelet commented Jun 3, 2024

Proposal for Basic Fastify API Architecture

This PR proposes a basic architecture for a Fastify API. It does not contain any features. I suggest we open an issue and create a roadmap for the next steps.

The base was generated by (and designed to work with) fastify-cli.

Core plugins used:
helmet, cors, swagger, swagger-ui, env, sensible, under-pressure, mysql

fastify-cli generates a lot of comments, but I decided to keep them because they are educational.
I've also added a server.ts file just to show how not to depend on fastify-cli to launch the app.

Wait for your feedbacks.

@melroy89
Copy link

melroy89 commented Jun 3, 2024

Maybe add helmet, cors, swagger and env plugin as well?

@melroy89
Copy link

melroy89 commented Jun 3, 2024

Also I would personally love to see typescript! Together with ESM.

@hpmouton
Copy link

hpmouton commented Jun 3, 2024

Is there perhaps a debug bar plugin for fastify which can give us metrics on the performance of our server,similar to the debug bar we see in the symfony demo and laravel framework? I can't seem to find one.
I think Typescript integration would be great.

On the note of the roadmap what is your initial ideas on the features and use cases of this project?

@jean-michelet
Copy link
Author

jean-michelet commented Jun 3, 2024

I'll add TypeScript tomorrow.

I'm not sure the way I've organized the plugins is relevant @melroy89.
I've tried to leverage the autoloader to keep the code clean, but I'd like your feedback.

Also, I'm reading Matteo's book on the subject of Fastify so I need to take more time to think about what I'm doing but the good news is I've got lots of time to work on this demo.

On the note of the roadmap what is your initial ideas on the features and use cases of this project?

I don't have many ideas, some sort of task app, or a blog/post might be a good start but I can't think of anything more fun and original.

I'm open to suggestions!

@melroy89
Copy link

melroy89 commented Jun 3, 2024

I'm not sure the way I've organized the plugins is relevant @melroy89.
I've tried to leverage the autoloader to keep the code clean, but I'd like your feedback.

Autoloader is good to use indeed! And also the best way of doing it IMO. I personally have my files within the plugins directory without the additional sub-directories now you created. Which I believe will reduce the complexity.

I personally added the Swagger fastify.register in server.ts. You created a separate plugin for only register the swagger plugin, I believe this is not necessary.

@melroy89
Copy link

melroy89 commented Jun 3, 2024

Is there perhaps a debug bar plugin for fastify which can give us metrics on the performance of our server,similar to the debug bar we see in the symfony demo and laravel framework? I can't seem to find one.

Not that I know of. I do know @fastify/under-pressure if you want to limit the calls. And in your case your are looking for fastify-metrics maybe to collect data and push it to Prometheus. And then use some Grafana instance for example to plot data.

But again, no built-in "debug bar" web UI element. I think fastify-metrics is the closest. And of course just use the default Pino logging??

@jean-michelet
Copy link
Author

Not that I know of. I do know @fastify/under-pressure if you want to limit the calls.

Ho, yep, I should add this plugin too I think.

@melroy89
Copy link

melroy89 commented Jun 4, 2024

@jean-michelet You are right on track with Type Providers for TS. Focused on schema validation, for both out- and incoming requests/responses. Do not forget an example of declaring your own version/extending the FastifyInstance interface (part of declare module 'fastify') for Fastify config options or other variables like DB (mysql).

Maybe also add a general example for authentication using @fastify/auth. Like maybe a simple but yet powerful JWT strategy on top of fastify/auth. Reminder fastify/auth is not providing an authentication strategy itself.

I believe another good practice is to configure setErrorHandler on the Fastify app. As well as setNotFoundHandler. My code:

    this.app.setErrorHandler((err, req, reply) => {
      req.log.error({ err })
      reply.status(err.statusCode || 500)
      reply.send(err)
    })

    this.app.setNotFoundHandler(async (req, reply) => {
      req.log.info({ req }, 'request was not found')
      reply.code(404)
      return { message: 'Not found' }
    })

Maybe you also want to add an caching example.

Last but not least, best practices for logging. Fastify() constructor provides a way to give additional options for the logger, but also ajv. Maybe ajv is out of scope, but very powerful non the less if you want to let's say remove additional body properties which aren't part of your schema you can configure that via { ajv: { customOptions: { removeAdditional: 'all' } }. And the logger via: { logger: <...>}. Options for the logger is either a boolean. Or during development I use a more advanced JS object that I pass to the logger:

 {
    level: 'info',
    transport: {
      target: 'pino-pretty',
      options: {
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname',
      },
    },
  }

Here a full example, what I have (do whatever you want with this info):

const environment = process.env.NODE_ENV || 'production'
 const envToLogger: {
     [key: string]: object | boolean
   } = {
     development: {
       level: 'info',
       transport: {
         target: 'pino-pretty',
         options: {
           translateTime: 'HH:MM:ss Z',
           ignore: 'pid,hostname',
         },
       },
     },
     production: true,
     test: false,
   }
   
   this.app = Fastify({
     logger: envToLogger[environment] ?? true,
     ajv: {
       customOptions: {
         coerceTypes: 'array', // change data type of data to match type keyword
         removeAdditional: 'all',// Remove additional body properties
       },
     },
   }).withTypeProvider<JsonSchemaToTsProvider>()

Ps. MAYBE... a MySQL or other database integration example? Like @fastify/mysql.

server.js Outdated Show resolved Hide resolved
app.js Outdated Show resolved Hide resolved
app.js Outdated Show resolved Hide resolved
server.js Outdated Show resolved Hide resolved
plugins/swagger.js Outdated Show resolved Hide resolved
@jean-michelet
Copy link
Author

I fixed the CI: https://github.com/jean-michelet/demo/actions/runs/9722290349/job/26835992622

Waiting for the merge or further review before continuing on this.
ci-base-architecture

@jean-michelet
Copy link
Author

Maybe just set a wildcard to ignore everything under scripts/*

Don't understand how to it.
I'm having rather a hard time with the Tap documentation, if someone would be kind enough to copy/paste the solution for me, I've spent too much time on it already.

@jean-michelet
Copy link
Author

I worked locally to add a rate limiter, but I think we should merge this before, otherwise I'll make your reviews obsolete.

@mcollina

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@melroy89
Copy link

melroy89 commented Jul 7, 2024

@Fdawgs you are now blocking the merge (request for change) ;). Is there still something we need to fix?

@Fdawgs
Copy link
Member

Fdawgs commented Jul 7, 2024

@Fdawgs you are now blocking the merge (request for change) ;). Is there still something we need to fix?

Just haven't had the time to review due to work being stupidly busy. Will take a look this coming week!

@melroy89
Copy link

melroy89 commented Jul 7, 2024

Ah no problem, thanks for your heads up.

src/app.ts Outdated Show resolved Hide resolved
src/server.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to explicitly register this inside each route and define the methods option to ensure each route returns the correct values in the Access-Control-Allow-Methods header.

Copy link
Author

@jean-michelet jean-michelet Jul 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to explicitly register this inside each route

Not sure to understand what you mean, the plugin is designed to be configured globally.

Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor tweaks.

Comment on lines 3 to 5
export const autoConfig: FastifyCorsOptions = {
methods: ['GET', 'POST', 'PUT', 'DELETE']
};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do a more precise configuration when we will set up the front end, cf #2

jean-michelet and others added 2 commits July 14, 2024 08:19
Co-authored-by: Frazer Smith <frazer.dev@icloud.com>
Signed-off-by: Jean <110341611+jean-michelet@users.noreply.github.com>
@melroy89
Copy link

Nice. It's approved! 🙏🎉. Well done @jean-michelet

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

Successfully merging this pull request may close these issues.

None yet

7 participants