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

Upgrade to TypeScript 5 #1194

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

Conversation

ArttuOll
Copy link
Contributor

ts-patch (the successor of ttypescript) has released a beta which adds support for TypeScript 5 and asked consumers to test it. In this branch, I've confirmed that this library works with the new version of ts-patch.

I've:

  • Replaced ttypescript with ts-patch (since ttypescript is not being developed anymore).
  • Fixed a type-narrowing-related bug in useEventListener that appeared with TypeScript 5.

This can be merged later, when the release version of ts-patch is available. I'll also format the commit messages then.

@ArttuOll ArttuOll changed the title Deps/upgrade to typescript 5 Upgrade to TypeScript 5 Apr 10, 2023
@xobotyi
Copy link
Contributor

xobotyi commented Apr 11, 2023

There is one thing i dont like about ts-patch - in some kind ts-patch breaks typecript itself - it is not something built around typescript like ttypescript, it injects into TS itself.

@xobotyi
Copy link
Contributor

xobotyi commented Apr 11, 2023

Otherwise - we're not in hurry here. TS 5 broken lots of things out there, i wast even able to switch any of projects at work, bcause of incompatibility.

@ArttuOll
Copy link
Contributor Author

There is one thing i dont like about ts-patch - in some kind ts-patch breaks typecript itself - it is not something built around typescript like ttypescript, it injects into TS itself.

The question I would ask now is: could we get rid of the dependency altogether? Is ttypescript really necessary?

@xobotyi
Copy link
Contributor

xobotyi commented Apr 11, 2023

Weeelp, with some tinkering around linting (that will enforce .js extension within imports) and TS configs - it is possible to get rid of plugins now. It was not, at the moment ttsc been introduced to our repos.

@xobotyi
Copy link
Contributor

xobotyi commented Apr 11, 2023

And maybe you're right.
Better move will be to get rid of plugins alltogether. It will require changing all imports and, i think, switch module resolution to ESNext (or maybe NodeNext)

@ArttuOll
Copy link
Contributor Author

And maybe you're right. Better move will be to get rid of plugins alltogether. It will require changing all imports and, i think, switch module resolution to ESNext (or maybe NodeNext)

Sounds good. I'm totally up for committing time to this.

dependabot bot and others added 3 commits April 15, 2023 18:51
Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.9.5 to 5.0.3.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](https://github.com/Microsoft/TypeScript/commits)

---
updated-dependencies:
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
@ArttuOll ArttuOll force-pushed the deps/upgrade-to-typescript-5 branch from afd996e to 89aee63 Compare April 15, 2023 16:12
@ArttuOll
Copy link
Contributor Author

I upgraded TypeScript, removed ttypescript, replaced ttsc with tsc in build scripts, changed module resolution to NodeNext and everything appears to work now.

@ArttuOll ArttuOll marked this pull request as ready for review April 15, 2023 16:15
@ArttuOll ArttuOll requested a review from xobotyi April 15, 2023 16:15
@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Merging #1194 (9af3992) into master (284e499) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
- Coverage   98.49%   98.40%   -0.10%     
==========================================
  Files          62       62              
  Lines        1064     1064              
  Branches      179      179              
==========================================
- Hits         1048     1047       -1     
- Misses          2        3       +1     
  Partials       14       14              
Impacted Files Coverage Δ
src/useEventListener/index.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xobotyi
Copy link
Contributor

xobotyi commented Apr 19, 2023

🤔 does it generate files that have .js imports?
im pretty much surprised that it dont require.js extension in imports

@xobotyi
Copy link
Contributor

xobotyi commented Apr 20, 2023

Well that was the point of plugin
We have to enforce extensions otherwise esm won't work.

@ArttuOll
Copy link
Contributor Author

The imports and requires of the generated files don't have any extensions (deleted the previous comment, because I built the wrong branch).

@ArttuOll
Copy link
Contributor Author

Interesting. I guess we'll have to go with ts-patch then? ttypescript and such won't work with TypeScript 5.

@xobotyi
Copy link
Contributor

xobotyi commented Apr 20, 2023

Nope
Just enforce it on linter level.

@xobotyi
Copy link
Contributor

xobotyi commented May 2, 2023

maybe we have to consider finally switch to "type": "module".
As for me we finally migrated all our production services to native esm.

@xobotyi
Copy link
Contributor

xobotyi commented May 14, 2023

so here's the plan.

  1. process current PR's
  2. migrate to new hooks testing library (@testing-library/react-hooks -> @testing-library/react)
  3. migrate to type: module
  4. migrate from storybook to docosurus (supposedly, not sure yet if docosaurus is viable with our directorues structure)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants