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

refactor: improve typing as required by tsc 5.3 or higher #186

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

berenddeboer
Copy link

Change Summary

The current library generates about 20 errors when included in a typescript project, possibly only when using TypeScript 5.3 or higher, see #183.

This is my first PR, a bit unclear about the process. For example "npm run format" formats a whole bunch of things, not related to my PR. So just stuck to what I guessed to be the code style in this library.

"npm build" generates errors about unused suppressions, not sure what the fix should be there. The suppressions are clearly needed, but the code could be improved to be more typesafe.

I'm also unsure if ImportError works correctly, it gets passed in two very different types, and it looks to me the array version is an error, but this is what the current code does.

PR Checklist

@berenddeboer
Copy link
Author

See also #181 which mentions ImportError as well as an issue.

@jasonbosco
Copy link
Member

Thank you for the PR @berenddeboer. This looks mostly fine to me. Could you address the issues reported in the CI pipeline?

@berenddeboer
Copy link
Author

Thanks @jasonbosco , unfortunately addressing the pipeline issues means removing the ts directive, which would make the library fail in newer typescript versions. The reason it reports this as unnecessary is because this is an older tsc.

@jasonbosco
Copy link
Member

Would upgrading the Typescript package in package.json to 5.3 help?

@berenddeboer
Copy link
Author

Would have sworn yes, but you are are right. It's the implicit any on tsconfig.json. Have forbidden this now, and now the error disappears. Not sure what you think about it, but imo disabling this forces people to think about types more.

@MikeAlexMartinez
Copy link

Is this still being worked on? I'm encountering the same issues and would like to fix this if possible

@berenddeboer
Copy link
Author

Up to @jasonbosco to decide if he wants to allow an implicit any or not. I fixed the issue at that time.

@MikeAlexMartinez
Copy link

I managed to fix my issues. I was importing the types from the src directory instead of the lib so it was reading lots checking the typesense project more than necessary.

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