Stricter ESLint configuration for code quality, consistency & cleaner merges #2343
Conversation
https://eslint.org/docs/latest/rules/prefer-const In our tests we use this pattern a lot: ``` let something; beforeEach(() => { something = ...; }); ``` This is not valid ~ prefer-const, but can be "let through" with some configuration. However, this would make the rule too loose for the non-test codebase. Therefore, we enable it overall but turn it off for tests only -- the `*.spec.ts` overrides apply to test files only and take precedence over `*.ts`
https://github.com/angular-eslint/angular-eslint/blob/main/packages/eslint-plugin-template/docs/rules/no-negated-async.md Prevents us from writing conditions that may pass/fail before an observable has emitted (this is likely to cause unexpected behaviour)
ybnd
changed the title
https://github.com/cartant/eslint-plugin-rxjs#rules Mainly to steer us away from unsafe patterns Skipping two recommended rules for now: rxjs/no-implicit-any-catch → not immediately clear how this should be addressed rxjs/no-nested-subscribe → there are quite a few cases, would take some effort to fix all of them
https://eslint.org/docs/latest/rules/comma-dangle Reduces merge conflict noise
Mainly to reduce merge conflict noise - Order by origin & alphabetically - Imports should be at the top of the file - Newline between imports & the rest of the file - Disallow duplicate imports - Enforce single-line when only importing one object - Enforce multi-line when importing two or more
TypeScript try/catch and RxJs catchError are not type-safe. https://typescript-eslint.io/rules/no-implicit-any-catch/ https://devblogs.microsoft.com/typescript/announcing-typescript-4-0/#unknown-on-catch https://github.com/cartant/eslint-plugin-rxjs/blob/main/docs/rules/no-implicit-any-catch.md
In most cases we can deal with the untyped errors by introducing an explicit instanceof check DspaceRestService includes an unsafe catch/rethrow → made it explicitly typed as `any`, but it should be revisited at some point
|
Hi @ybnd, |
|
@tdonohue @atarix83 @artlowel
|
- Introduced pipes for combined null/undefined checks: dsHasValue & dsHasNoValue - Safer to async !== true than async === false - Went over other instances of async === to confirm that they should be fine
|
Hi @ybnd, |
|
(force pushed last two commits because I'd committed a temporary Karma config change by accident) |
There was a problem hiding this comment.
@ybnd: I'm reviewing/testing this today. Overall, it looks good, and I'm not seeing any changes in behaviour when running the UI in production mode (yarn build:prod; yarn serve:ssr). However, I am seeing that all HTTP errors (404s, etc) are now logged in great detail in the SSR logs (in console where you ran yarn serve:ssr) and in the browser DevTools Console. Is there maybe a stray console.log in this PR (I'm having difficulty finding it myself)? Somehow this PR makes those logs a lot more noisy.
Beyond that, I haven't found any other users, but I'm still testing various areas of the application.
|
@ybnd : Regarding my prior comment, I think the stray Once that log is removed, I'm +1 this PR. I haven't found any obvious bugs or changes in behavior in my testing today. |
|
@tdonohue thanks for pointing that out, Ok apparently the code used to only log errors for the This made everything more noisy because the
Out of scope for a linting PR, so I restored the original behaviour. |
ybnd
force-pushed
the
more-eslint
branch
2 times, most recently
from
fdb1914 to
0f2e776
Compare
When refactoring to meet the rxjs/no-implicit-any-catch ESLint rule, I'd made it so errors were logged for DspaceRestService.request calls as well, making it a lot more noisy The console.log in .get is explicitly required by tests though
tdonohue
dismissed
atarix83’s stale review
Verified that all feedback was addressed. If further cleanup is needed it can be done in a follow-up PR
|
Merging this immediately in order to free up work on #2750 (as discussed in yesterday's DevMtg). @ybnd : Now that this is merged, can you please help to enhance/update the Code Style Guide (Angular section) with details about the new style rules from this PR? At a minimum, I think we should document the following:
If you don't feel comfortable updating this wiki page, you can add notes in a comment here & I'll copy them over. In the meantime, I'll flag this as |
tdonohue
added
the
needs documentation
Task/dspace cris 2023 02 x/DSC-1823 Approved-by: Andrea Barbasso
References
Description
This PR makes our lint configuration a bit stricter, most importantly:
@typescript-eslint/ban-types→ disallow confusing/harmful TypeScript types@angular-eslint/template/no-negated-async→ disallow!(value$ | async)in Angular templates@angular-eslint/template/eqeqeq→ disallow!=and==in Angular templatesindent&comma-danglefor reduced merge conflict noiseInstructions for Reviewers
Review whether the changes to the ESLint configuration are sensible
Confirm whether the stricter rules are worth the increased linting time (from ~2 minutes to ~3 minutes)
The app (and the tests) should build and run in exactly the same way as before.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
yarn lintyarn check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.