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

Add initial typescript config and use it for eslint,vitest,playwright #31186

Merged
merged 24 commits into from
Jun 28, 2024

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented May 30, 2024

This enables eslint to use the typescript parser and resolver which brings some benefits that eslint rules now have type information available and a tsconfig.json is required for the upcoming typescript migration as well. Notable changes done:

  • Add typescript parser and resolver
  • Move the vue-specific config into the root file
  • Enable vue-scoped-css/enforce-style-type rule, there was only one violation and I added a inline disable there.
  • Fix new lint errors that were detected because of the parser change
  • Update i/no-unresolved to remove now-unnecessary workaround for the resolver
  • Disable i/no-named-as-default as it seems to raise bogus issues in the webpack config
  • Change vitest config to typescript
  • Change playwright config to typescript
  • Add eslint-plugin-playwright and fix issues
  • Add tsc linting to make lint-js

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 30, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 30, 2024
@silverwind silverwind force-pushed the tsconfig branch 4 times, most recently from 4894af3 to d31eb63 Compare May 30, 2024 18:05
@silverwind silverwind changed the title Add initial typescript config for eslint Add initial typescript config and use it for eslint/vitest May 30, 2024
@silverwind silverwind changed the title Add initial typescript config and use it for eslint/vitest Add initial typescript config and use it for eslint,vitest,playwright May 31, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/docs labels May 31, 2024
@lunny
Copy link
Member

lunny commented Jun 2, 2024

Should we really want to switch to typescript?

@silverwind
Copy link
Member Author

silverwind commented Jun 2, 2024

Should we really want to switch to typescript?

Definitely. It'll catch a number of undiscovered bugs. Initially it will just be a rename of the files with non-enforced type checking, and we can then gradually fix the type issues.

I don't think we will ever enable strict typescript (not worth the effort imho and I personally do that only on a very small amount of my modules), so people can still write it mostly like JS, with optional type info sprinkled in.

@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 Jun 2, 2024
* origin/main: (21 commits)
  Fix deprecated Dockerfile ENV format (go-gitea#31450)
  README Badge maintenance (go-gitea#31441)
  Improve markdown textarea for indentation and lists (go-gitea#31406)
  Split common-global.js into separate files (go-gitea#31438)
  Fix the link for .git-blame-ignore-revs bypass (go-gitea#31432)
  Bump htmx to 2.0.0 (go-gitea#31413)
  Fix the wrong line number in the diff view page when expanded twice. (go-gitea#31431)
  Fix labels and projects menu overflow on issue page (go-gitea#31435)
  [Fix] Account Linking UpdateMigrationsByType  (go-gitea#31428)
  Fix markdown math brackets render problem (go-gitea#31420)
  Reduce `air` verbosity (go-gitea#31417)
  Fix new issue/pr avatar (go-gitea#31419)
  Increase max length of org team names from 30 to 255 characters (go-gitea#31410)
  [skip ci] Updated translations via Crowdin
  Refactor names (go-gitea#31405)
  Update JS dependencies, remove `eslint-plugin-jquery` (go-gitea#31402)
  Switch to upstream of `gorilla/feeds` (go-gitea#31400)
  Fix rendered wiki page link (go-gitea#31398)
  Refactor repo unit "disabled" check (go-gitea#31389)
  Refactor route path normalization (go-gitea#31381)
  ...
@silverwind
Copy link
Member Author

@wxiaoguang any objections to this?

@wxiaoguang
Copy link
Contributor

No idea, not an expert in this field. Maybe you could ask @go-gitea/technical-oversight-committee

@silverwind
Copy link
Member Author

silverwind commented Jun 28, 2024

Given that this PR has received no attention in a month, I assume there is no interest in typescript among the maintainers, which is a shame as I've recently come to like it a lot and it has lead me to find hundrets of bugs in my various JS projects.

Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

I am for TS over JS, though you and wxiaoguang are definitely the ones who touch it the most in this project.
I'll give it a LGTM since you are interested in moving to it and wxiaoguang at least appears to be neutral on the idea? (Feel free to correct me)

@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 Jun 28, 2024
@silverwind
Copy link
Member Author

silverwind commented Jun 28, 2024

Thanks for reassuring. I assume @wxiaoguang will also eventually appreciate typescript.

It personally took me 5 years and I strongly rejected earlier, but after using it for a while now, I don't want to miss it anymore especially because of the nice editor integration that properly typed code has.

@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 28, 2024
@silverwind silverwind enabled auto-merge (squash) June 28, 2024 15:47
@wxiaoguang
Copy link
Contributor

Thanks for reassuring. I assume @wxiaoguang will also eventually appreciate typescript.

It personally took me 5 years and I strongly rejected earlier, but after using it for a while now, I don't want to miss it anymore especially because of the nice editor integration that properly typed code has.

I also use TS in my projects, but I didn't see how it would help Gitea's state and I didn't see any plan or guideline. I have shared my opinions in another PR: starting from the hardest part and rewriting some existing code could show more than just proposing.

@wxiaoguang
Copy link
Contributor

I have shown the similar question for "jQuery removal". But it seems that replacing jQuery functions with a lot of $xxx[0]?.yyy just decreases the maintainability IMO. I think the question is the same to "TS introducing": if it is mixed with existing code or abused, it still decreases the maintainability.

@wxiaoguang
Copy link
Contributor

I am for TS over JS, though you and wxiaoguang are definitely the ones who touch it the most in this project.

Thank you for the feedback, I will be pretty busy in next months so I guess I won't have time in this project then 😁

@silverwind silverwind merged commit 08579d6 into go-gitea:main Jun 28, 2024
26 checks passed
@silverwind
Copy link
Member Author

silverwind commented Jun 28, 2024

Thanks for reassuring. I assume @wxiaoguang will also eventually appreciate typescript.
It personally took me 5 years and I strongly rejected earlier, but after using it for a while now, I don't want to miss it anymore especially because of the nice editor integration that properly typed code has.

I also use TS in my projects, but I didn't see how it would help Gitea's state and I didn't see any plan or guideline. I have shared my opinions in another PR: starting from the hardest part and rewriting some existing code could show more than just proposing.

The plan is to rename *.js to *.ts, post a small guidline and then gradually fix the type issues. It's not rocket science. TS is for all intends and purposes a superset of JS and I'm sure our code will work with just a rename.

@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 28, 2024
@wxiaoguang wxiaoguang deleted the tsconfig branch June 28, 2024 16:17
@wxiaoguang
Copy link
Contributor

The plan is to rename *.js to *.ts, post a small guidline and then gradually fix the type issues. It's not rocket science.

Yup, "jQuery removal" is also not rocket science, but I have spent so much time on fixing regressions.

@wxiaoguang
Copy link
Contributor

To be clear: I never meant I have objection for TS or "jQuery removal", instead I appreciate everyone's work. I just mean: it should be done with some plans and guidelines, to avoid abuse.

@silverwind
Copy link
Member Author

I'd say it's easier to migrate to typescript than to migrate off jQuery because jQuery has so much magic behaviour internally.

With typescript fixes, I'm usually pretty confident because the added code never actually reaches the runtime.

@silverwind
Copy link
Member Author

Next step: #31521

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/docs modifies/go Pull requests that update Go code modifies/internal modifies/js size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/code-linting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants