fastify / demo

MIT License
10 stars 3 forks source link

Create basic architecture #1

Closed jean-michelet closed 1 month ago

jean-michelet commented 3 months ago

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 commented 3 months ago

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

melroy89 commented 3 months ago

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

hpmouton commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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.

jean-michelet commented 3 months ago

Should we use ts-standard? I use it on my own projects, but the rules are pretty strict. For example, explicit-function-return-type is applied.

melroy89 commented 3 months ago

Should we use ts-standard? I use it on my own projects, but the rules are pretty strict. For example, explicit-function-return-type is applied.

I'm using eslint and the official typescript-eslint plugin: https://typescript-eslint.io/getting-started/. I use eslint together with @typescript-eslint/recommended" and prettier.

More info about typescript eslint: https://typescript-eslint.io/

melroy89 commented 3 months ago

I have another general remark; move all the code (except maybe the test code? Leave that in test) to a sub-directory called src. I personally do not like to have my code in the root-folder next to the README.md, instead I move the code into a separate directory like src.
Update the package.json file (or other configs) accordingly if needed.

jean-michelet commented 3 months ago

Current TODO for setting up basic architecture:

If anyone has feedbacks, I'll continue to work on this next week.

@metcoder95 maybe?

jean-michelet commented 2 months ago

In order to keep a simple CI, I think we should only run the tests on Linux. It's a demo, not a dependency so I think it's ok. linux-only

jean-michelet commented 2 months ago

I left a pair of comments, so far I believe we can maybe simplify it a little and focus a bit more on the documentation that highlights the pieces of fastify.

Yes, I haven't really thought about the documentation yet. I had planned to split the creation of this repository into several PRs to give interested developers the opportunity to contribute. But I think we should not create something too simple, it's a great opportunity to show implementations with advanced parts of Fastify and Node.js.

Regarding the development process, which do you prefer @mcollina @metcoder95? I can also work until version 0 is ready, I'm fine working on my own ;)

jean-michelet commented 2 months ago

Regarding the development process, which do you prefer @mcollina @metcoder95? I can also work until version 0 is ready, I'm fine working on my own ;)

If no answer, I will start to create a API.

Not sure what I should use for authentication though... I think it's a bad idea to use JWT by default, challenging to implement securely imo. At the same time, many Node.js tutorials use it...

metcoder95 commented 2 months ago

We can keep it on v0 for a while until we feel comfortable with it.

metcoder95 commented 2 months ago

Not sure what I should use for authentication though... I think it's a bad idea to use JWT by default, challenging to implement securely imo. At the same time, many Node.js tutorials use it...

Can be a good default, tho it is important to document that this is just a demo and further security recommendations should be added

jean-michelet commented 2 months ago

Can be a good default, tho it is important to document that this is just a demo and further security recommendations should be added

Can be a good opportunity to educate about security too, but I understand that it could be out of scope.

jean-michelet commented 2 months ago

I'm working on the design of the api, a basic task application. I'd like to manage authorization with user permissions. I want to add the ability to add avatars to users and task generation in CSV format to show how to manipulate files.

This could be a good start, I haven't thought much about what to do next, but do you have any ideas? Any plugin suggestions?

jean-michelet commented 2 months ago

For now, the tests will not pass. Can you activate the CI @mcollina?

I identified some stuff to fix, but a merge doesn't involve official communication, and it would allow others to contribute.

jean-michelet commented 2 months ago

@fastify/collaborators

A code review would be appreciated.

jean-michelet commented 2 months ago

From metcoder95, regarding the doc:

It will be nice here to develop a bit more about the methodology or decisions made that lead to this repository; that backstory sometimes helps companies to decide wether or not to adopt a given technology (I've faced that in the past)

OK, but it would be nice if, even if I'm the one who suggested it, other developers could share their perspectives.

jean-michelet commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

@mcollina

melroy89 commented 2 months ago

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

Fdawgs commented 2 months ago

@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 commented 2 months ago

Ah no problem, thanks for your heads up.

melroy89 commented 1 month ago

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

jean-michelet commented 1 month ago

Can it be merged plz? I would like to continue.

mcollina commented 1 month ago

You should now have enough permissions to land it yourself!

Go ahead, lgtm!