Migrate icu macro to typescript #1870
Conversation
| "import": "./icu.macro.d.mts" | ||
| }, | ||
| "default": "./icu.macro.js" | ||
| "default": "./dist/commonjs/icu.macro.js" |
There was a problem hiding this comment.
we may streamline this like the other exports... import, require, etc... commonjs and es
There was a problem hiding this comment.
happy to do this, just want to make sure the general approach is OK. Outstanding questions:
- should I migrate the whole repo to TS? This would be easy, but be thousands of lines of new code to review
- should I instead migrate the work I did in typescript down to javascript? I found several subtle issues in the babel macro that only TS types caught which is why I went with this approach
Once I have clarity on those, I'll finish up. I just got tests/linting to pass (yay!)
There was a problem hiding this comment.
Since react-i18next was always a JavaScript module and only later shipped with types (but still having javascript code), I would say we stay with JavaScript, until there's a real need to move everything to TypeScript.
There was a problem hiding this comment.
I would gently encourage thinking in terms of 2 things:
- safety of contributions
- AI-generated code
AI is waaaay more accurate with typescript than javascript, because it gets much more accurate feedback on its changes. Going forward, you're going to see probably all of your contributions come from folks working with their robot overlords, and the quality will be much higher with a strongly typed language like Typescript.
To illustrate that in a more realistic fashion, in the PR you can't see yet (depends on TS existing here), which introduces an entirely new Trans component under the hood for icu macros, I found a ton of edge cases in the babel API by using typescript, things that are potentially the cause of bugs I have observed repeatedly in production at my company with the current implementation. These bugs are incredibly difficult to reproduce, as they only occurred when the Trans was at the end of a very long file, and removing code before the Trans would eliminate the bug. This meant that I can't ever do a bug report here, because I would have to include proprietary code just to reproduce the bug.
Moving to Typescript is painless, and I could have it all done for the entire repo in 1 day if that is desired.
Sticking with Javascript is always possible, but again continues the risk profile that limits the future potential for safe iteration.
I defer to you all, these are just my reasoned arguments for moving to TS.
There was a problem hiding this comment.
Thanks a lot for the detailed explanation and all the effort you’ve put into this.
I totally get your points — TypeScript can definitely help during development by catching subtle issues early and making both human and AI-assisted contributions safer and more predictable. That’s a real advantage on the developer experience side.
That said, at runtime there’s no difference — everything ends up as plain JavaScript, so from a functional or behavioral perspective, TypeScript doesn’t change the actual code execution.
And regarding AI contributions, the quality should ideally be the same either way as long as the project is well-structured and covered by tests.
For this PR, I’d prefer to keep the focus on the Trans/ICU topic and stay with JavaScript for consistency with the rest of the repo.
We can absolutely have a separate, focused discussion later about whether migrating the entire repo to TypeScript makes sense strategically — but I’d rather not mix that larger question into this specific change.
There was a problem hiding this comment.
ok, I'll close this PR and do the TS -> JS on the other contribution. Thanks!
|
Is it really necessary to introduce TypeScript code? With that now we have mixed JS and TS... |
|
btw: tests are failing |
I am willing to migrate the entire thing to TS, was just trying to limit the scope of this PR. As for why, making a new macro with TS is far safer than making it without. I developed the new component also using TS, which made it far safer. An alternative approach I am willing to do is to take the component I made and the new macro and strip the typescript and add new javascript in the repo. |
4 participants
This is a preparatory step to adding a new
Transcomponent which can be used for the icu macro. It converts the existing icu macro to Typescript, and adds tests to the existing macro. Referencing issue: #1869To make things easier to modify later, it splits up the file into many separate files, and adds tests for each specific chunk.
The existing test was moved to typescript, but the snapshot is identical to the old one, showing that it is exactly the same code
Checklist
npm run testChecklist (for documentation change)