search

NEAR-Edu / near-certification-tools

2 stars 2 forks source link

[WIP] Use eslint-config-near #78

Closed petarvujovic98 closed 2 years ago

petarvujovic98 commented 2 years ago

Install eslint-config-near and fix errors Seed script: Extract some common logic and fix ESlint errors

render[bot] commented 2 years ago

Your Render PR Server URL is https://near-certification-tools-pr-78.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-caot2s0nlki9t9rj8360.

ryancwalsh commented 2 years ago

@petarvujovic98 I've never seen this type of thing. Can you explain? https://github.com/NEAR-Edu/near-certification-tools/pull/78/files#diff-95756e4df5b75ac564995b645060410c84ee95714a7d7c96ae72cbfa94ce3fe5R2-R3 and https://github.com/NEAR-Edu/near-certification-tools/pull/78/files#diff-dc722112229a01bfc10aad136f1d28ab416de2acb8ea1c5e6752ca5422d80ea5R3-R7

My IDE generally says return await is redundant. Thoughts? https://github.com/NEAR-Edu/near-certification-tools/pull/78/files#diff-dc722112229a01bfc10aad136f1d28ab416de2acb8ea1c5e6752ca5422d80ea5R59

Is there some new rule that automatically alphabetizes properties? Is that definitely desirable? https://github.com/NEAR-Edu/near-certification-tools/pull/78/files#diff-d1df2a59a3a0afbf45f655f84f3b8f451c837a921ba50aafb6536b80b5bccfe5R2-R18

petarvujovic98 commented 2 years ago

@ryancwalsh So in terms of the type annotation, in JavaScript files it serves a purpose of telling the IDE you use to search for that particular type/interface and associate it with the variable below so that you could get completions and autosuggestions just like you would when using TypeScript files, and in the TypeScript file imports sections it merely tells the reader that the import is just a type or interface and doesn't stay there after compiling to JavaScript.

As for return await it is redundant, but also doesn't actually change the flow of execution or performance, the logic behind this rule is to let the reader know that the called function returns a Promise and should be awaited even though it has no effect on that particular call where it is directly returned.

Yes, there is a rule that alphabetizes properties. I personally like it and it means that keys in objects will always be sorted in an alphabetic order so that it is more predictable when reading through the code.

Of course these are just my opinions about those rules, so if you have a reason for turning them off we can discuss that. The return await one I don't have a strong preference for nor against, I just understand why it could be useful so maybe we could remove it.

ryancwalsh commented 2 years ago

@petarvujovic98 https://stackoverflow.com/questions/44806135/why-no-return-await-vs-const-x-await is a question about https://eslint.org/docs/latest/rules/no-return-await . I'm not 100% clear on the trade-off of the tiny performance benefit that we'd be sacrificing by disabling this no-return-await rule, but I don't feel strongly either way. I think answers at https://stackoverflow.com/questions/43353087/are-there-performance-concerns-with-return-await lean towards your approach (and seem to suggest that Eslint is mistaken).

in the TypeScript file imports sections it merely tells the reader that the import is just a type or interface

Ahh, it's for the benefit of the human reader. Interesting.

Thanks.

This PR looks good to go.