Migrate to ESLint and apply Airbnb JavaScript Style Guide #150
Conversation
| @@ -0,0 +1,5 @@ | |||
| module.exports = { | |||
| singleQuote: true, | |||
| printWidth: 100, | |||
There was a problem hiding this comment.
Can we remove printWidth option here? (and leave the default value)
There was a problem hiding this comment.
The rule being followed here is https://airbnb.io/javascript/#whitespace--max-len.
There was a problem hiding this comment.
Yeah, but at the same time, 120 is the default option proposed by prettier. So far, we've used prettier's one so I think that we shouldn't change this.
|
|
||
| @Controller() | ||
| export class AppController { | ||
| export default class AppController { |
There was a problem hiding this comment.
Default exports are not recommended with Nest and should be permitted rather than suggested
There was a problem hiding this comment.
The rule being followed here is https://airbnb.io/javascript/#modules--prefer-default-export.
| getHello(): string { | ||
| return 'Hello World!'; | ||
| return this.helloWorld; |
There was a problem hiding this comment.
Can we revert this change to stay consistent with all tutorials & guides & courses on the web?
There was a problem hiding this comment.
So you would like to turn off this eslint rule?
https://eslint.org/docs/rules/class-methods-use-this
|
Concerning the changes in the code style. I've adjusted the code to follow the rules of the Airbnb Style Guide. When adopting a style guide, I highly recommend not tweaking the rules of the guide to fit personal preferences. The whole point of adopting a guide is consistency and to get rid of bikeshedding. Many organizations and projects use a style guide and are intransigent with their specified rules. Google is an example of such organization:
Source: https://google.github.io/eng-practices/review/reviewer/standard.html |
|
@IGassmann I haven't proposed changes that are in line with my preferences, but the framework recommendations according to the documentation and the entire ecosystem. |
|
|
||
| async function bootstrap() { | ||
| async function bootstrap(): Promise<void> { |
There was a problem hiding this comment.
Promise<void> doesn't bring any relevant & useful information here. I know that it's forced by the Style Guide, but I doubt we should follow this. TypeScript type inference is sufficient in such scenarios.
@kamilmysliwiec Yes. I should have written preferences in general. The point I'm trying to communicate is the value of having a documented style guide that we can use as a reference and not break its rules. That would mean updating the style of the projects and documentation to align with the style of the guide. Another proposition I would have if we want to keep some existing styles is to write a Nest Style Guide and build an ESLint plugin or adopt another style guide that is closer to Nest's style and also has the tools ready. Maybe Google's one would be a viable solution. It is adopted by the Angular projects. Personally, I would recommend adopting Airbnb's one, even if that means slightly changing the current style of the projects. I believe the internet doesn't need one more JavaScript style guide and we would be able to take advantage of the already present tools around this style guide. |
|
It doesn't make too much sense to create a dedicated style guide if we just want to override a few rules to make all repos & documentation consistent. I do agree with the majority of AirBnb rules, although, some decisions aren't the best and we shouldn't just stick to it because the inherited style guide enforces them. Both eslint and tslint allow to override rules and that's what makes them powerful. Obviously, we should reduce the number of them as much as possible, but people eventually will use either our CLI, typescript-starter, samples from the main repo or docs to start their projects. If we ensure that all these repos share the same eslint file with the same set of rules - that would be totally fine, regardless of a couple of modified rules. |
|
@kamilmysliwiec Yes, we could override those rules and we would still solve the problem of consistency since projects would share the same rules. However, by overriding those rules we would sacrifice the value of having a fully documented style guide. Because our rules wouldn't be anymore following the Airbnb ones completely. That also gives precedence to allow more divergence and discussion about rules in the future. The value of a documented style guide is that it reduces bikeshedding. If in the future someone starts a trivial argument about one style or another, we can point to the documentation and indicate that it is the source of truth. There the person will find examples and the reason behind the rule. Then we can spend more time on things that matter more. Let me know if the value of the documentation makes sense to you. If you feel strong about the currently used style preferences, we could also fork the Airbnb Style Guide and write our own guide. |
|
As I said above:
Regarding this:
I don't have anything against custom rules. As I mentioned here:
Every organization can override rules, adjust them to their preferences - and it's totally fine. What we can do, as a framework, is to remain consistent across resources that we provide (CLI, samples, repos, documentation) regardless of whether we will override 0 or 5 rules from the original doc. Furthermore, if anyone wants to override our rules/add even more rules within the organization, it's not a big deal. We provide recommendations, suggestions, and they will be clearly visible in all locations + editable from the configuration files. However, we cannot force them to use these rules anyway. To recap, we definitely won't create our own style guide because that would create even more divergence in the ecosystem. At the same time, I don't see any issue in overriding a few rules which are useless, especially this one with default imports which has been in doubt (even for AirBnb style guide users) for a long time. |
|
Thanks for keeping answering. I appreciate the discussion. I will try to restate what you're saying to be sure I'm understanding your thinking. Please correct me, if I misinterpreted you. As I understand, you're okay with us (the Nest community) not having a documented style guide that serves as a source of truth that we can point to. If that is right, does my bikeshedding argument makes sense to you?
Totally agree with you here. I'm not suggesting to enforce organizations to follow our style.
Agreed. That's why I believe it is better to follow the Airbnb one. But to be clear, by follow, I mean follow strictly. If we a break a few rules, the guide ceases to be a source of truth and then all rules can be debated, and we're back at bikeshedding.
The issue being is that we lose the source of truth. I also wouldn't say that they necessarily are useless. Although, the value of some of those rules is an endless debate. I personally prefer, named exports for example. But there are good arguments for preferring default exports. But it is most of the time an unproductive discussion to debate about these things. That's where one of the main values of a style guide resides, like well explained by Standard JS's FAQ:
|
|
@kamilmysliwiec wouldn't it make more sense to wait until Angular migrates to ESLint? NestJS can run in an Angular project, therefore we would want to have the same ESLint ruleset. I'd rather wait until they have published one before we start doing our own thing. angular/angular#13732 |
|
@BrunnerLivio Nest can also run in React and Vue project 😄 Anyway, I think I got what you mean. We can wait a little to see which route Angular will take for some additional inspiration. |
|
@BrunnerLivio @kamilmysliwiec makes sense for me to wait for Angular's migration. They will probably set it up with the Google JavaScript Style Guide rules, which is also a well-documented style guide. |
|
@IGassmann you going to good direction, but airbnb use react where |
|
@pumano yes, I could see how Google's style guide would be a better fit if the Nest wants to align with the Angular project. However, Google's style guide has some questionable decisions too like requering JSDoc for all classes, methods, and functions. Other things are also different from what Nest has been doing, like asking for annotation to be written in camelCase. One could also argue that Nest is agnostic of frontend frameworks, and then there's no necessity for it to be aligned with Angular style. |
|
Hey guys, shall we finalize this? If there's a debate in which one to use, Google or Airbnb, you can refer to this article: https://medium.com/@uistephen/style-guides-for-linting-ecmascript-2015-eslint-common-google-airbnb-6c25fd3dff0 or some others around there. I personally go with Airbnb since it's the most popular one and also since it have more React support. But it's up-to you. If @kamilmysliwiec want to modify it, I'm completely ok with it since it does look like this framework need some modifications. But the thing is, tslint have announce their deprecation February this year. Now we have waited too long for it's adoption which is not good at all. We can't wait for Angular guys. So, shall we come to any common agreement and move forward with this? @IGassmann If you need any help I'm free. 🙂 Will it be ok if I help with the changes? |
|
I believe it's still okay for us to wait. TSLint is still accepting PRs for bug and security fixes. However, I also understand the need to get this done, so new NestJS projects won't need to make the switch to ESLint themselves. The angular project might be preparing an ESLint configuration that would accommodate their style guide, which ain't exactly the Google one. So it might be useful for us to follow this new configuration. As a note, we have adopted internally the Airbnb Style Guide since September for our NestJS project. It's been working fine, even though there are some divergences with the Angular rules, such as the use of default exports. |
|
@IGassmann No offense but this is not an Angular project. This is a back-end framework used by many other developers as well. I agree this project follow many good standards set by Angular. But that does not mean we need to follow them in all the other cases. Also note that Angular focus on Front end and this focus on the Back end. |
|
@THPubs no offense taken. I'm bringing Angular, because the project was inspired by it and many have come from it. I personally don't use Angular. In my own opinion, I would adopt Airbnb, or create a custom one, but have it documented somewhere by forking the documentation of an existing one. |
|
@THPubs there is a good chance we won't follow exactly the same set of rules as Angular. However, since they already started working on this, it would be great to ensure our ESLint configurations are as close as possible to each other. Let's be patient, they should release it shortly. |
|
Any update bro? It's 2020 now. |
|
Switching to ESLint is a forced measure because TSLint will not be updated soon (sadly, ESLint is much slower). But Airbnb rules are a bad idea, I think. It has a lot of front-end specific rules and it doesn't let you work quietly. Instead of focusing on writing code, you have to worry about satisfying Airbnb rules. Can we make our own rules for Nest based on Airbnb experience? |
|
I would extend airbnb as it nearly became a convention to do it. Then I just disabled the unsatisfying rules. I am now using the same eslint config for my front-end and back-end without any conflict. |
|
Extend/Fork Airbnb's would be a good strategy. I would also add TypeScript specific rules since Airbnb targets only JavaScript. Nest would then have its own set of rules and style guide like other frameworks out there. |
|
Let's track this here #161 Initially, we wanted to wait until Angular publishes a new release. Unfortunately, Angular 9 (new major release) is already a few months delayed, and the migration (from tslint to eslint) has been postponed until Angular 10. More information in the PR. |
This PR migrate from TSLint to ESLint since the former has been deprecated.
It also configures ESLint and Prettier to align themselves with the Airbnb JavaScript Style Guide, which is the most popular style guide. It is well documented and its rules have been thoroughly discussed by the community. Thanks to
typescript-eslintit can also be used with TypeScript.After merging this, the idea is to normalize the linting configuration between the different Nest official projects and documentation as discussed here.