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

Fix remaining typescript issues, enable tsc #32840

Merged
merged 17 commits into from
Dec 15, 2024
Merged

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Dec 14, 2024

Fixes 79 typescript errors. Discovered at least two bugs in notifications.ts, and I'm pretty sure this feature was at least partially broken and may still be, I don't really know how to test it.

After this, only like ~10 typescript errors remain in the codebase but those are harder to solve.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 14, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 14, 2024
@silverwind silverwind added type/refactoring Existing code has been cleaned up. There should be no new functionality. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Dec 14, 2024
@silverwind silverwind changed the title Fix a number of typescript issuest Fix a number of typescript issues Dec 14, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 14, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 14, 2024

There should be no "tsc" error now

tested: dropzone, tribute, repo search (explore page)

@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Dec 14, 2024
@silverwind
Copy link
Member Author

silverwind commented Dec 14, 2024

Those .vue import errors can be fixed by using vue-tsc instead of tsc, but it's currently broken for typescript 5.7 which we already use: vuejs/language-tools#5018.

As for the not callable and not constructable errors in dynamic imports, I'm out of ideas but it must be something with the ts configuration I assume.

@silverwind
Copy link
Member Author

silverwind commented Dec 14, 2024

I guess I will just uninstall vue-tsc and we live with those few @ts-expect-error. vue-tsc is essentially monkeypatching tsc and that will likely break again in the future, so I think we are better off using official typescript tooling only.

@silverwind
Copy link
Member Author

silverwind commented Dec 14, 2024

tsc is now enabled as part of make lint-js and is passing for me and should do too on CI.

Edit: Yes, it's passing, so this is ready.

@silverwind silverwind changed the title Fix a number of typescript issues Fix remaining typescript issues, enable tsc Dec 14, 2024
@silverwind
Copy link
Member Author

I switched back to vue-tsc, to a fork of mine that is from their master branch which works with typescript 5.7, but for some reason was not released yet (see vuejs/language-tools#5018).

.vue files are getting type-checked again and the errors on the imports on .vue files go away. Interestingly a new error shows in vitest.config.ts, so I added a @ts-expect-error there. I hope we can resolve this later.

@silverwind
Copy link
Member Author

FYI I opened vitejs/vite-plugin-vue#487 regarding this not callable error.

@silverwind
Copy link
Member Author

silverwind commented Dec 15, 2024

So I think I found the magic incantation that requires the least amount of @ts-expect-error. It's from the vite vue+ts template:

    "module": "esnext",
    "moduleResolution": "bundler",

This is a bit suboptimal as we do have a few node.js scripts, but we can likely split the tsconfig into two later, one for node files and one for web_src, but I will defer that to another PR amd this is ready to merge.

@silverwind
Copy link
Member Author

silverwind commented Dec 15, 2024

I added type definitions for all named exports from *.vue files, so npx tsc is now also clean. tsc does not typecheck the .vue files because it can not parse them, so we still need to use vue-tsc for that.

Long-term, I'd like to forbid named exports from Vue SFCs, they just don't seem worth it when these functions can easily live outside. Should be possible with eslint.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 15, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 15, 2024
@lunny lunny merged commit c8ea41b into go-gitea:main Dec 15, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 15, 2024
@silverwind silverwind deleted the types-9 branch December 16, 2024 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/dependencies modifies/frontend modifies/internal size/L Denotes a PR that changes 100-499 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants