search

onur-ozkan / nestjs-rate-limiter

Highly configurable and extensible rate limiter library
https://npmjs.com/package/nestjs-rate-limiter
MIT License
238 stars 43 forks source link

Check in first example NestJS Controller App #48

Closed shannonlal closed 3 years ago

shannonlal commented 3 years ago

The following PR is a proposal for expanding the examples directory (#26). I am proposing creating a NestJS sample app within example directory. Here is what is included in this PR:

  1. Using the NestJS I created a new app within the examples directory
  2. I have modified the default controller to reference to the parent dist directory so it will always pull in the latest build code from the parent directory

Potential Issues

  1. Ideally it would not need a separate package.json within the examples I was having issues trying to get nestjs to work correctly but it was not able to work within the examples directory. In the future it may have to be refactored to use something like lerna and nx as a a monorepo

Value of this PR

  1. If we have an example project within the main repo it could be useful for testing different features of the library
  2. It provides a series of examples that other developers can look at for guidance
  3. Because the sample project references the parent (rate-limiter directory) it could be included within the build pipeline. We could use something like postman collections (with newman) to run full integration tests. I think this would be useful for trying to move towards a full CI/CD pipeline.

@ozkanonur I welcome any comments/suggestions and refactoring. If you feel this is not the right way to do this you can reject the PR as well.

onur-ozkan commented 3 years ago

@shannonlal Thanks again! I actually wanted to make multiple example project below the /examples directory. Like rate-limiter-express-app, rate-limiter-fastify-app, rate-limiter-graphql-app. Can you wrap your example project with the directory called rate-limiter-yourtech-app ?

shannonlal commented 3 years ago

@ozkanonur Not a problem but I wanted to ask a couple of questions. If we are going to have multiple apps within the examples directory would you be open to using something like nx for a monorepo strategy? It has a lot of plugins with nestjs and would be useful for sharing code and a consistent package.json. Here are a couple of options I am thinking of:

1. No mono repo

/examples --/rate-limiter-express-app/ --/rate-limiter-fastify-app/ --/rate-limiter-graphql-app/ -- etc

2. Mono Repo for just examples /examples/ package.json nx.json jest.config.js .. /apps/ ---- NOTE: All apps would share the common package.json and same config files --/rate-limiter-express-app/ --/rate-limiter-fastify-app/ --/rate-limiter-graphql-app/

3. Mono Repo for everything package.json nx.json jest.config.js /lib/ -- Current rate-limiter-code. .. /apps/ ---- NOTE: All apps would share the common package.json and same config files --/rate-limiter-express-app/ --/rate-limiter-fastify-app/ --/rate-limiter-graphql-app/

Question for @ozkanonur . Which of the following would you be open to? My recommendation is number 2 as it starts moving in the correct direction. Number 3 is probably the better option in the long term but I may need some help setting up the lib directory and making sure the build artifact works.

onur-ozkan commented 3 years ago

@shannonlal Sorry I couldn't focus and read the PR correctly, working so hard on some other projects these days. I think something like nx could be nice so yes number 2 looks also good to me. It's much better to deal with workspaces than dealing with directories.

shannonlal commented 3 years ago

@ozkanonur this is a larger PR than I would normally like but it includes a couple of basic things. I removed the existing examples directory and created a simple nest.js workspace with one app. I have only added one default controller but was able to link the controller back to the parent dist directory so we can test changes in the nestjs-rate-limiter project to see how they would impact the different test apps.

Here are the nest steps I am proposing:

  1. Expand the number controllers in the express app to demonstrate the different options with rate limit
  2. Reproduce the changes with fastify
  3. Reproduce the changes with graphql.

If you are okay with the following I will start making small issues and PR to expand on the following.

onur-ozkan commented 3 years ago

@shannonlal Yes, I am okay with those steps. Sorry for the late response.

shannonlal commented 3 years ago

@ozkanonur Would you be open to approving this PR? It is already fairly large and I tend to make small PRs so it is easier to review. If you are okay to merge this I will start making smaller PRs based on the items above