patrickpissurno / fastify-esso

The easiest authentication plugin for Fastify, with built-in support for single sign-on (SSO)
https://npm.im/fastify-esso
MIT License
52 stars 6 forks source link

Configuration Improvements and Type fixes #1

Closed dodiego closed 4 years ago

dodiego commented 4 years ago

The project doesn't extend the original fastify typings, which causes type errors when tryting to reference:

This PR address those issues and adds some others improvements to the project configuration

patrickpissurno commented 4 years ago

Hi,

Thanks for your contribution. Before I can merge your PR, I need you to undo the "added eslint with standardjs" part of it.

I can merge all the other changes, but eslint totally broke the diff and changed the coding style of the source code.

I do see the reasoning for eslint in large codebases, where many devs have direct push access to the upstream repo. But there are shortcomings with it too, and I don't see it as a good fit for a repo this size. I didn't write a "CODESTYLE.md" file, because it's just not necessary. The codebase is small, and all contributions go through me, so I can still double-check (just as I'm doing right now).

Please format you code with 4 spaces, instead of 2. And use simple quotes (') instead of double quotes("). You'll have to undo the changes eslint has done. That's why it's nice to do only one thing per commit. This way it's a lot easier to undo only one part of it.

If I may, I'd recommend making a copy of your modified source code in a separate folder, then reverting to the upstream head with git --reset-hard, then copying only what you really changed back (and ajusting the code style to match).

Take your time to do the changes. And again, thank you for your contribution. Better IDE support is really nice and important. It'll make this plugin much more usable.

dodiego commented 4 years ago

Hey @patrickpissurno, just updated the files without the eslint styling, it is much cleaner and easier to review it now.

I still believe that the project can benefit from some styling enforcement, I had to disable my code formatter because it doesn't depend on eslint rules and other developers might encounter the same problem when contributing to the project, but I still agree with you that this PR should not do that.

I can make another PR with some eslint/prettier configuration respecting the current code styling (single quotes, semicolons, trailing commas, etc), that way we keep the code as it is and anyone who contributes will not have to worry with that.

Also, great work with this project!

patrickpissurno commented 4 years ago

You're right.

I'll be merging this now.

Thanks!

patrickpissurno commented 4 years ago

This feature is already out on NPM version 1.1.0 of fastify-esso! npm i --save fastify-esso@latest