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.

Sign up for GitHub

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

Jump to bottom

Improving the overall type safety of the entire library #2917

Closed
wants to merge 3 commits into from
Closed

Improving the overall type safety of the entire library #2917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2315003
Changed tsconfig rule set for stricter typings
Idono87 May 5, 2021
f968bfb
Jest is now type checking code
Idono87 May 5, 2021
65cb40a
Readded skipLibCheck option
Idono87 May 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ module.exports = {
moduleNameMapper: pathsToModuleNameMapper(tsconfig.compilerOptions.paths, { prefix: `${ROOT_DIR}/` }),
collectCoverage: false,
cacheDirectory: resolve(ROOT_DIR, `${CI ? '' : 'node_modules/'}.cache/jest`),
preset: 'ts-jest',
Copy link
Owner

@ardatan ardatan May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I avoided to use ts-jest on purpose in order to have faster compilation and reduce overhead of tsc. I don't think we need type checking in tests. At least, it is not CI's job I believe. So it's done by babel already. I thought of removing ts-jest completely but I realized that we still need it for pathsToModuleNameMapper above to use paths from tsconfig file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree that for most cases the CI should not have to run type checking. But for open source tools and libraries with advanced generic types like the ResolversComposerMapping type, they should absolutely be tested . They are still a written piece of code and should be treated as such since they are prone to errors like any other code.

Tsc and babel only validate that types are correct. They don't know how to test intended behavior within generic types and ultimately allows code like this to pass type checking

interface Resolvers {
  Query?: {
    foo: () => string
  }
}

export const rcMap: ResolversComposerMapping<Resolvers> = {
  Query: {
    foo: 'hello'
  }
}

I understand that faster compile times and less overhead is desired. But if it compromises type robustness then it's not a great trade-off and makes using said types superfluous since they might or might not work as intended.

};
6 changes: 1 addition & 5 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,7 @@
"declaration": true,
"downlevelIteration": true,

"suppressImplicitAnyIndexErrors": true,
"noImplicitAny": true,
"alwaysStrict": true,
"noUnusedLocals": true,
"noUnusedParameters": true,
"strict": true,
"skipLibCheck": true,
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure if running type check for dependencies is a good idea. This might be overhead for our build process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an oversight by me. Thank you for spotting it.


"paths": {
Expand Down