FEI-4957.4: Run codemod to convert files to TypeScript #526
Conversation
🦋 Changeset detectedLatest commit: 7fd0012 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
@kevinbarabash If you don't have a task for it yet, you're going to want to add a TS linter step that runs on a repo (probably have it check to see if a flowconfig exists, then run flow, and if a tsconfig exists, run TypeScript) |
I don't have anything in the migration guide about setting up GitHub actions. I'll add a section to call that out after I finish converting this repo. It's all going to be under the same task. |
There was a problem hiding this comment.
Good to see the automation do so much work for us. It's a shame that so many error suppressions get added when a judicial use of any or mixed would fix it (specifically regarding tests).
I am a little concerned by the suppressions in some of the more strongly-typed functions like clone.js, but we can revisit these things post-conversion, and they seem to be specific to the internals rather than leaking into the external representation, so no big deal.
|
|
||
| // Act | ||
| const result = cloneMetadata(metadata); | ||
| // @ts-expect-error [FEI-5011] - TS2540 - Cannot assign to 'string' because it is a read-only property. |
There was a problem hiding this comment.
note: It's a shame we have all these error suppressions instead of just a judicial use of any on the variable from the beginning. Since these are test files, that feels like the right thing. Of course, that's manual work and would really take time, so I'm not requesting a change here. Just lamenting the tech debt/mess that is left behind.
There was a problem hiding this comment.
I expect there will be quite a bit of post-conversion work to improve our types.
| const originalValue = { | ||
| array: [1, 2, 3], | ||
| }; | ||
| } as const; |
There was a problem hiding this comment.
question: What is as const for? Why is it needed if the variable is already const? Is it because TS demands that both sides of the = are const'd?
There was a problem hiding this comment.
I'm not sure why the tool adds as const in so many places. We could change that behavior. as const in this context changes the type that is inferred for the object literal. With as const, it's inferred as precisely {array: [1, 2, 3]} whereas without it, it's inferred as {array: number[]}. TS playground example
kevinbarabash
force-pushed
the
fei-4957.3
branch
from
acc55bd to
de6491e
Compare
kevinbarabash
force-pushed
the
fei-4957.4
branch
from
aef01f1 to
5a08264
Compare
kevinbarabash
force-pushed
the
fei-4957.3
branch
from
de6491e to
055ad4a
Compare
kevinbarabash
force-pushed
the
fei-4957.4
branch
from
5a08264 to
24ba309
Compare
kevinbarabash
force-pushed
the
fei-4957.4
branch
from
24ba309 to
7fd0012
Compare
I think we might be able to solve this with conditional types to help feed more accurate type info through the body of the function so that we don't need the |
Summary:
This changeset was generated using the follow commands:
The next PR will fix all the TypeScript errors
Issue: FEI-4957
Test plan: