-
Notifications
You must be signed in to change notification settings - Fork 75
The Great AntAlmanac TypeScript Rewritening™
https://github.com/icssc/AntAlmanac/pull/411
This is a dev log about rewriting our project in TypeScript. So far the only author is @EricPedley.
- First thing I did was follow the Create-React-App tutorial for installing TypeScript on our project: https://create-react-app.dev/docs/adding-typescript/
Why TypeScript? - It makes the developer experience easier. With TypeScript, we get to know exactly what to pass as parameters to a function or what fields an object has with IDE autocomplete, and you get compiler errors for messing anything up instead of it silently failing until it shows up as a runtime error, which is a big problem for us because we don't have unit tests to automatically catch those.
-
Ran the local dev server to see if everything compiled. It did locally but not on our staging environment because of a peer dependency conflict between the TypeScript version in our package and the one that
react-scripts
depends on, so I updatedreact-scripts
and luckily it fixed it. -
Renamed a single file to
.tsx
and fixed the resulting compile errors. Making changes one at a time like this will temporarily mess up the git history, but once we squash and merge it will be reduced to one commit. We can add that commit to our.git-blame-ignore-revs
, which tellsgit blame
what commits to ignore, so we can ignore that one commit.
the difference between the
.ts
and.tsx
files is the presence of the embedded HTML-in-JS snippets like and such, which are called Components in React.
- Realized the huge scope of this task and split it up into 4 smaller tasks to complete, each for rewriting a different section of the codebase. This will still end up fine because we can merge them into the parent PR and then squash&merge that one, making the whole rewrite only 1 commit that we can ignore on git blame.
Getting involved with more complicated details.
- Just had a tough time rewriting the ConditionalWrapper component in TS, hoping to use this example to illustrate how to read TS error messages. Read it and then read on. The error message looks intimidating but when you break it down is actually makes a lot of sense when we break it down.
The | (pipe) character between types, like
Element | ReactElement
is a type union, meaning that either Element or ReactElement are valid. In a parameter type annotation, it means you can pass either as args and it will work. In a return type annotation, it means that you can safely assume the function will return one or the other of those types.
-
First line: The part I underlined in red is just the type structure of the arrow function we're passing as an argument to forwardRef. The part underlined in green is the type that forwardRef expects as its parameter.
-
Second line: This is explaining why the red and green types are incompatible. Their return types are different. The blue box and underline are the return type of the function we passed, and the yellow underline is the expected return type of the function we pass to forwardRef.
-
Third line: This explains why the function return types differ. The blue one has
Element
in the union, while the second one doesn't. This means that our function might return an Element, which is somehow incompatible with ReactElement. If we hover overwrapper(children)
, it tells us exactly why they're incompatible (underlined in pink): ReactElement has some properties Element doesn't such asprops
. So, if forwardRef's code calls our function and tries to access theprops
value of the returned object, it might be undefined, so it doesn't let us possible pass a value like that. So, the fix? Change the type of children to ReactElement to guarantee it has all the right properties.
-
This is harder than I thought it would be. I'm having trouble rewriting the News component because its state has two properties that are initialized to null, and later in functions it has checks for if those properties are non-falsy and their length is not zero, but the problem is that since they're intiialized to undefined, the compiler thinks that the second part that checks the length is unreachable, so it assigns the length property the 'never' type since it thinks that it won't be able to get it because that state value is undefined, even though it is updated in another function to be not undefined. Using null gives the same problem. Might have to resort to posting on StackOverflow for this one because I'm not finding existing answers, or rewrite the component as a functional component instead of a class component.
-
I just type-annotated state directly on the line it was initialized and it fixed the problem. I was already passing in an interface for state as a parameter to PureComponent but I guess the type system didn't infer that
state
was of that type, so I just needed to explicitly say it. Another problem I spent a lot of time on was where ConditionalWrapper wasn't letting me pass an array of elements into it as a child, and I was confused because the specific array I was passing also contained ConditionalWrappers so I didn't see that the error was coming from the parent conditional wrapper. Once I read that, the error made sense that I was passing an array of elements, not a single one, so it couldn't cast right. I fixed it by just wrapping the list in a fragment.
Before:
After:
And all the references to state worked without any further code changes (except renaming eventName
to title
for consistency) 😎
Can you tell what's wrong with this code? It's not immediately clear anything is wrong, until we add TYPESCRIPT
The string for the action is being passed to the label prop instead of the action prop like it should be. With TS, it's an easy catch. I wonder who wrote this buggy code in the first place ... 😳
Edit: after scrolling through this again I spotted a mistake: when I was resetting the code back to JS for the first screenshot I even mistyped label
as line
, and that didn't give me any errors since the file was in JS. I probably would've caught it manually testing with the console open but catching it sooner is always better.
Picture this: a deeply nested child component needs access to some variable in an ancestor component. Let's say we have a structure like A -> B -> C -> D where A is the parent of B, and so on. The easy way, prop drilling, is for A to pass the variable as a prop to B, and B to C, and C to D. This can get confusing sometimes, so prop drilling is generally considered bad practice. If you google the term, most articles are about how to avoid it.
So, our site has a button to screenshot your schedule from the calendar view.
That component is nested like so (the box colors are referring to the annotated screenshot below):
- ScheduleCalendar (red box)
- CalendarPaneToolbar (yellow box)
- ScreenshotButton (green box)
- CalendarPaneToolbar (yellow box)
Now, the ScreenshotButton is kinda normal, it just has an onClick prop for the Button element inside itself. But looking closely you see it's pretty convoluted:
handleClick is a function defined in ScreenshotButton that actually creates the png and downloads it. But, that's passed to a function that it receives via props from CalendarPaneToolbar:
And onTakeScreenshot is both the name of ScreenshotButton's parameter we're passing this arrow function to, as well as the name of a prop being passed to CalendarPaneToolbar. Basically, all this arrow function does is call logAnalytics and then pass handleClick up to a handler in its parent function:
This function takes the screenshot function as a parameter. It edits the canvas temporarily and then actually takes the screenshot, then resets the canvas to its original state. Basically I'm trying to say this was a pain in the ass to figure out and should probably be refactored at some point.
This error (screenshot below) came up while working on converting ScheduleNameDialog to ts. Didn't take long for me but I can see how this could block someone if they don't know what dev tools to use.
The first thing I try for these is a ctrl-f
for the function name to see where it's being used for hints. I then hover over the prop it's being passed to to see if the prop type tells me what kind of function it's expecting. In this case, no such luck because the parameter is any
type (see below). If it isn't clear, my mouse is hovered over the word onClick
for that dialog to pop up in VSCode.
Next, I use this crazy hack to coerce IntelliSense into telling me what the function type is:
Putting the function inline for some reason makes it work, and it tells me the function type. Then I just copy/paste that to the definition in our class, and it fixes the error. In my case, I also removed one of the types from the union because I knew it would be a TextArea specifically.
Bam, compiler error fixed!
While writing the CourseDetailView component in TS I tried to use my editor autocomplete to auto-import the CustomEvent type I had defined earlier and I realized I had defined it in two places, one where the start and end times were strings, and one where they were Date
objects. Uh-oh! The problem is that custom events only ever show the date visually, so it's using string, while the course events actually use the properties of a date object.
Ahh, it gets worse! To keep track of what day of the week the event is on, one CustomEvent uses a boolean array, while the other one just adds multiple instances for each day of the week the event is on.
The biggest problem here is that one CustomEvent takes its start and end time from an <input type=time ...>
element and the other one gets it from who-knows-where but I did some sophisticated investigation (console.log) and found out it is indeed using Date
objects. I'm just going to do the lazy solution and rename one of the CustomEvents and add comments on both clarifying their properties.
The PeterPortal API is also written in TypeScript, so I can just steal their types for the API responses. I straight-up copypasted their type files into AntAlmanac and it makes life so much easier. I was just converting a component that took meetings
as a prop, so I took a guess that there was an interface named Meeting to use, and just wrote it, let VSCode auto-import it from the types file, and it just worked without any looking through files or console logging anything. The feeling when it just works without any extra work is extremely satisfying.
Congrats on reaching the bottom! If you were curious about the background images in the code screenshots, they're frames from the movie 5 Centimeters per Second displayed with the VSCode background-cover extension.