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

Improved jsdoc/type-checking setup #120

Merged
merged 14 commits into from
Jan 6, 2025
Merged

Conversation

elpoelma
Copy link
Collaborator

@elpoelma elpoelma commented Dec 9, 2024

Overview

This PR improves the jsdoc/type-checking setup of this javascript service.
Specifically it sets up:

  • A more strict tsconfig.json
  • Addition of the typescript dev-dependency
  • Addition of some jsdoc types
  • Inclusion of @ts-nocheck in files with missing/incorrect types
  • A slight refactor of the woodpecker set-up

How to test/reproduce

  • Both the typescript compiler and eslint should not fail
  • When integrated in this stack, this service should still behave the same functionally

Challenges/uncertainties

I included two logic changes:

  • Moved prefixMap from a Map to a readonly Record to get better types
  • Fix a bug where the incorrect translation key was being used.
  • This set-up will allow us to make this service gradually more type-safe

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

@elpoelma elpoelma force-pushed the internal/type-checking branch from b9ef70b to 5b65bc4 Compare December 9, 2024 20:54
@elpoelma elpoelma changed the base branch from master to internal/update-node December 9, 2024 20:55
@elpoelma elpoelma marked this pull request as ready for review December 9, 2024 21:04
@elpoelma elpoelma added the enhancement New feature or request label Dec 9, 2024
@elpoelma elpoelma self-assigned this Dec 9, 2024
Copy link
Collaborator

@lagartoverde lagartoverde left a comment

Choose a reason for hiding this comment

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

looks good, added some prefixMap conversions you forgot :)

Copy link
Collaborator

@piemonkey piemonkey left a comment

Choose a reason for hiding this comment

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

Hmm, given the need to disable TS on so many files to avoid strict mode errors, I think it would make sense for us to try https://github.com/allegro/typescript-strict-plugin , which should allow us to toggle strict mode per file, and so still get limited typechecking in those files.

@elpoelma
Copy link
Collaborator Author

Hmm, given the need to disable TS on so many files to avoid strict mode errors, I think it would make sense for us to try https://github.com/allegro/typescript-strict-plugin , which should allow us to toggle strict mode per file, and so still get limited typechecking in those files.

This indeed sounds like a good idea. I'll check it out and see what impact it has on performance of the language server. There's also a draft PR open to add support for this in typescript: microsoft/TypeScript#49886

@elpoelma elpoelma force-pushed the internal/type-checking branch from 24bfa5f to 234b951 Compare December 13, 2024 10:16
@elpoelma elpoelma force-pushed the internal/type-checking branch from 4b366f3 to 0f6d1f6 Compare December 13, 2024 10:26
@elpoelma elpoelma force-pushed the internal/type-checking branch from 0f6d1f6 to 7d41932 Compare December 13, 2024 10:27
@elpoelma
Copy link
Collaborator Author

@piemonkey I added the plugin and was able to replace most @ts-nocheck statements by @ts-strict-ignore. There are still some places (e.g. the test files) where the @ts-nocheck was also necessary.

@elpoelma elpoelma requested a review from piemonkey December 13, 2024 10:30
Copy link
Collaborator

@piemonkey piemonkey left a comment

Choose a reason for hiding this comment

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

Looks great :)

Base automatically changed from internal/update-node to master January 6, 2025 10:38
@piemonkey piemonkey merged commit 0ba4a50 into master Jan 6, 2025
2 checks passed
@piemonkey piemonkey deleted the internal/type-checking branch January 6, 2025 10:38
@elpoelma elpoelma added internal and removed enhancement New feature or request labels Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants