Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(chore) Extend ESLint configuration #902

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Conversation

denniskigen
Copy link
Member

Requirements

Summary

In this PR, I've extended the ESLint configuration used in this project as follows:

  • Include recommended rules from ESLint itself, Prettier, and TypeScript ESLint.
  • Enable the @typescript-eslint/consistent-type-imports rule to enforce consistent usage of the import type syntax for type imports.
  • Configure the no-console rule to throw an error for any console statement except those using warn and error.
  • Add some useful TypeScript rules which are initially turned off to keep the diff in this PR manageable.

I've made some other tangential changes, including:

  • Renaming extensions for resource files that don't contain JSX from .tsx to ts.
  • Replacing pretty-quick with lint-staged as our pre-commit hooks library because pretty-quick is not compatible with the latest versions of prettier.

Screenshots

None

Related Issue

None

Other

None

Copy link
Contributor

github-actions bot commented Dec 11, 2023

Size Change: -283 kB (-9%) ✅

Total Size: 2.83 MB

Filename Size Change
packages/esm-appointments-app/dist/755.js 0 B -48.1 kB (removed) 🏆
packages/esm-patient-registration-app/dist/643.js 0 B -117 kB (removed) 🏆
packages/esm-patient-registration-app/dist/975.js 0 B -35.6 kB (removed) 🏆
packages/esm-patient-search-app/dist/853.js 0 B -25.2 kB (removed) 🏆
packages/esm-service-queues-app/dist/257.js 0 B -52.7 kB (removed) 🏆
ℹ️ View Unchanged
Filename Size Change
packages/esm-active-visits-app/dist/130.js 107 kB 0 B
packages/esm-active-visits-app/dist/255.js 2.21 kB 0 B
packages/esm-active-visits-app/dist/277.js 12.4 kB 0 B
packages/esm-active-visits-app/dist/316.js 42.9 kB 0 B
packages/esm-active-visits-app/dist/319.js 631 B 0 B
packages/esm-active-visits-app/dist/382.js 1.15 kB 0 B
packages/esm-active-visits-app/dist/460.js 727 B 0 B
packages/esm-active-visits-app/dist/574.js 553 B 0 B
packages/esm-active-visits-app/dist/588.js 6.66 kB 0 B
packages/esm-active-visits-app/dist/629.js 50.9 kB 0 B
packages/esm-active-visits-app/dist/635.js 1.15 kB 0 B
packages/esm-active-visits-app/dist/729.js 3.1 kB 0 B
packages/esm-active-visits-app/dist/757.js 649 B 0 B
packages/esm-active-visits-app/dist/784.js 2.63 kB 0 B
packages/esm-active-visits-app/dist/788.js 551 B 0 B
packages/esm-active-visits-app/dist/807.js 864 B 0 B
packages/esm-active-visits-app/dist/833.js 669 B 0 B
packages/esm-active-visits-app/dist/879.js 2.94 kB 0 B
packages/esm-active-visits-app/dist/main.js 67.6 kB 0 B
packages/esm-active-visits-app/dist/openmrs-esm-active-visits-app.js 3.33 kB 0 B
packages/esm-appointments-app/dist/130.js 107 kB 0 B
packages/esm-appointments-app/dist/152.js 257 B 0 B
packages/esm-appointments-app/dist/255.js 2.23 kB 0 B
packages/esm-appointments-app/dist/303.js 258 B 0 B
packages/esm-appointments-app/dist/319.js 1.99 kB 0 B
packages/esm-appointments-app/dist/460.js 2.18 kB 0 B
packages/esm-appointments-app/dist/469.js 6.65 kB 0 B
packages/esm-appointments-app/dist/549.js 246 kB 0 B
packages/esm-appointments-app/dist/574.js 1.74 kB 0 B
packages/esm-appointments-app/dist/591.js 16.9 kB 0 B
packages/esm-appointments-app/dist/610.js 6.71 kB 0 B
packages/esm-appointments-app/dist/729.js 3.1 kB 0 B
packages/esm-appointments-app/dist/757.js 1.75 kB 0 B
packages/esm-appointments-app/dist/784.js 2.63 kB 0 B
packages/esm-appointments-app/dist/788.js 1.75 kB 0 B
packages/esm-appointments-app/dist/799.js 48.1 kB 0 B
packages/esm-appointments-app/dist/80.js 2.52 kB 0 B
packages/esm-appointments-app/dist/807.js 2.4 kB 0 B
packages/esm-appointments-app/dist/833.js 1.99 kB 0 B
packages/esm-appointments-app/dist/main.js 298 kB +1 B (0%)
packages/esm-appointments-app/dist/openmrs-esm-appointments-app.js 3.29 kB +3 B (0%)
packages/esm-patient-list-management-app/dist/130.js 107 kB 0 B
packages/esm-patient-list-management-app/dist/255.js 2.21 kB 0 B
packages/esm-patient-list-management-app/dist/319.js 1.5 kB 0 B
packages/esm-patient-list-management-app/dist/382.js 1.15 kB 0 B
packages/esm-patient-list-management-app/dist/460.js 1.69 kB 0 B
packages/esm-patient-list-management-app/dist/493.js 21.6 kB 0 B
packages/esm-patient-list-management-app/dist/497.js 104 kB 0 B
packages/esm-patient-list-management-app/dist/574.js 1.31 kB 0 B
packages/esm-patient-list-management-app/dist/588.js 6.66 kB 0 B
packages/esm-patient-list-management-app/dist/591.js 16.9 kB 0 B
packages/esm-patient-list-management-app/dist/635.js 1.15 kB 0 B
packages/esm-patient-list-management-app/dist/716.js 4.65 kB 0 B
packages/esm-patient-list-management-app/dist/729.js 3.1 kB 0 B
packages/esm-patient-list-management-app/dist/757.js 1.47 kB 0 B
packages/esm-patient-list-management-app/dist/784.js 2.64 kB 0 B
packages/esm-patient-list-management-app/dist/788.js 1.3 kB 0 B
packages/esm-patient-list-management-app/dist/807.js 1.81 kB 0 B
packages/esm-patient-list-management-app/dist/833.js 1.55 kB 0 B
packages/esm-patient-list-management-app/dist/main.js 129 kB 0 B
packages/esm-patient-list-management-app/dist/openmrs-esm-patient-list-management-app.js 3.3 kB 0 B
packages/esm-patient-registration-app/dist/130.js 107 kB 0 B
packages/esm-patient-registration-app/dist/152.js 262 B 0 B
packages/esm-patient-registration-app/dist/255.js 2.21 kB 0 B
packages/esm-patient-registration-app/dist/303.js 260 B 0 B
packages/esm-patient-registration-app/dist/319.js 1.74 kB 0 B
packages/esm-patient-registration-app/dist/438.js 35.6 kB 0 B
packages/esm-patient-registration-app/dist/460.js 1.79 kB 0 B
packages/esm-patient-registration-app/dist/492.js 118 kB 0 B
packages/esm-patient-registration-app/dist/537.js 2.34 kB 0 B
packages/esm-patient-registration-app/dist/574.js 1.49 kB 0 B
packages/esm-patient-registration-app/dist/591.js 16.9 kB 0 B
packages/esm-patient-registration-app/dist/62.js 6.86 kB 0 B
packages/esm-patient-registration-app/dist/729.js 3.1 kB 0 B
packages/esm-patient-registration-app/dist/735.js 464 B 0 B
packages/esm-patient-registration-app/dist/757.js 1.74 kB 0 B
packages/esm-patient-registration-app/dist/784.js 2.64 kB -1 B (0%)
packages/esm-patient-registration-app/dist/788.js 1.42 kB +5 B (0%)
packages/esm-patient-registration-app/dist/807.js 2.2 kB 0 B
packages/esm-patient-registration-app/dist/833.js 1.75 kB 0 B
packages/esm-patient-registration-app/dist/879.js 2.94 kB 0 B
packages/esm-patient-registration-app/dist/884.js 6.1 kB 0 B
packages/esm-patient-registration-app/dist/main.js 156 kB +947 B (+1%)
packages/esm-patient-registration-app/dist/openmrs-esm-patient-registration-app.js 3.33 kB +1 B (0%)
packages/esm-patient-search-app/dist/130.js 107 kB 0 B
packages/esm-patient-search-app/dist/152.js 261 B 0 B
packages/esm-patient-search-app/dist/255.js 2.21 kB 0 B
packages/esm-patient-search-app/dist/303.js 260 B 0 B
packages/esm-patient-search-app/dist/319.js 936 B 0 B
packages/esm-patient-search-app/dist/382.js 1.15 kB 0 B
packages/esm-patient-search-app/dist/423.js 25.2 kB 0 B
packages/esm-patient-search-app/dist/460.js 1.06 kB 0 B
packages/esm-patient-search-app/dist/574.js 781 B 0 B
packages/esm-patient-search-app/dist/588.js 6.66 kB 0 B
packages/esm-patient-search-app/dist/591.js 16.9 kB 0 B
packages/esm-patient-search-app/dist/635.js 1.15 kB 0 B
packages/esm-patient-search-app/dist/729.js 3.1 kB 0 B
packages/esm-patient-search-app/dist/757.js 938 B 0 B
packages/esm-patient-search-app/dist/784.js 2.63 kB 0 B
packages/esm-patient-search-app/dist/788.js 776 B 0 B
packages/esm-patient-search-app/dist/807.js 1.11 kB 0 B
packages/esm-patient-search-app/dist/832.js 26 kB 0 B
packages/esm-patient-search-app/dist/833.js 964 B 0 B
packages/esm-patient-search-app/dist/main.js 54.4 kB +1 B (0%)
packages/esm-patient-search-app/dist/openmrs-esm-patient-search-app.js 3.34 kB +1 B (0%)
packages/esm-service-queues-app/dist/130.js 107 kB 0 B
packages/esm-service-queues-app/dist/152.js 262 B 0 B
packages/esm-service-queues-app/dist/255.js 2.23 kB 0 B
packages/esm-service-queues-app/dist/303.js 261 B 0 B
packages/esm-service-queues-app/dist/319.js 3.14 kB 0 B
packages/esm-service-queues-app/dist/328.js 3.08 kB 0 B
packages/esm-service-queues-app/dist/389.js 2.42 kB 0 B
packages/esm-service-queues-app/dist/425.js 2.06 kB +1 B (0%)
packages/esm-service-queues-app/dist/460.js 3.98 kB 0 B
packages/esm-service-queues-app/dist/469.js 6.66 kB 0 B
packages/esm-service-queues-app/dist/574.js 3.14 kB 0 B
packages/esm-service-queues-app/dist/591.js 16.9 kB 0 B
packages/esm-service-queues-app/dist/610.js 6.71 kB 0 B
packages/esm-service-queues-app/dist/616.js 2.71 kB -1 B (0%)
packages/esm-service-queues-app/dist/694.js 2.64 kB 0 B
packages/esm-service-queues-app/dist/729.js 3.1 kB 0 B
packages/esm-service-queues-app/dist/733.js 0 B -2.64 kB (removed) 🏆
packages/esm-service-queues-app/dist/757.js 3.14 kB 0 B
packages/esm-service-queues-app/dist/784.js 2.63 kB 0 B
packages/esm-service-queues-app/dist/788.js 3.13 kB 0 B
packages/esm-service-queues-app/dist/794.js 161 kB 0 B
packages/esm-service-queues-app/dist/807.js 4.37 kB 0 B
packages/esm-service-queues-app/dist/833.js 3.63 kB 0 B
packages/esm-service-queues-app/dist/86.js 0 B -2.42 kB (removed) 🏆
packages/esm-service-queues-app/dist/89.js 52.7 kB 0 B
packages/esm-service-queues-app/dist/981.js 2.92 kB 0 B
packages/esm-service-queues-app/dist/main.js 217 kB -1 B (0%)
packages/esm-service-queues-app/dist/openmrs-esm-service-queues-app.js 3.31 kB -1 B (0%)

compressed-size-action

@denniskigen denniskigen force-pushed the chore/lint-consistent-type-imports branch from fa814c9 to d560aef Compare December 11, 2023 06:45
@jwnasambu
Copy link
Contributor

jwnasambu commented Dec 11, 2023

Wow! @denniskigen this LGTM! I believe its gonna save us on the errors we have been struggling with locally.

Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @denniskigen! It's so nice to see efforts toward making our codebase cleaner.

I'm curious about why we're excluding test files from these rules. Personally, I lean towards having consistent rules across both application and test files.

@denniskigen
Copy link
Member Author

That's fair, @jayasanka-sack. I think I only plopped that in there because we historically omitted tests from linting. I've removed ignorePatterns from the config and amended the eslintignore file appropriately. There is one stylistic concern about the consistent-type-imports rule syntax that's currently being discussed here. Let's see whether we can arrive at a consensus which we'll then replicate here.

* (chore) Bump testing dependencies and fix test console warnings

* Remove unused file from registration app tsconfig

* Commit some orphaned translations

* Cleanup

* Fixup

* Review feedback - fixup

* Update playwright version in e2e Dockerfile
@denniskigen denniskigen force-pushed the chore/lint-consistent-type-imports branch from 5adef9d to 3e8a9a9 Compare December 21, 2023 19:48
@denniskigen
Copy link
Member Author

denniskigen commented Dec 21, 2023

Should be good to go, @jayasanka-sack @ibacher. The PR in Core mentioned above got merged.

@ibacher
Copy link
Member

ibacher commented Dec 21, 2023

I'd tend to be more on board with updating the linting rules and then applying them to tests in another PR, but since it's done, it's done (as a rule, its better to have many smaller commits than a few large commits and it's better to have commits that make as small a change as possible at a time).

Thanks @denniskigen!

@ibacher ibacher merged commit 308d628 into main Dec 21, 2023
6 checks passed
@ibacher ibacher deleted the chore/lint-consistent-type-imports branch December 21, 2023 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants