refactor(types): Set up type checking #427
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| "@typescript-eslint/eslint-plugin": "^2.23.0", | ||
| "@typescript-eslint/parser": "^2.23.0", | ||
| "bittorrent-tracker": "^9.19.0", | ||
| "browserslist": "^4.21.9", |
There was a problem hiding this comment.
This was yelling at me while running tests, so I updated the config. This shouldn't affect too much.
| export const getPlotImage = (plotContent, x, y) => { | ||
| if (plotContent) { | ||
| if (getPlotContentType(plotContent) === itemType.CROP) { | ||
| export const getPlotImage = (plotContents, x, y) => { |
There was a problem hiding this comment.
This function needed the most refactoring, but it should be functionally the same as before.
| if (!seedItemId) | ||
| throw new Error( | ||
| `Crop item ID ${cropItemId} does not have a corresponding seed` | ||
| ) |
There was a problem hiding this comment.
This error should never be thrown in practice, but it was necessary for this function to be type safe. I expect we'll need to use this pattern more as we adopt Typed JavaScript.
| /** | ||
| * @param {(?farmhand.plotContent)[][]} field | ||
| * @param {function(?farmhand.plotContent): boolean} condition | ||
| * @returns {?farmhand.plotContent} | ||
| */ | ||
| (field, condition) => { | ||
| const [plot = null] = | ||
| field.find(row => { | ||
| return row.find(condition) | ||
| }) ?? [] |
There was a problem hiding this comment.
This function was actually functionally broken before, as it didn't return what I intended. It simply hadn't manifested in a noticeable way yet. Typed JavaScript exposed a preexisting bug! 😃
There was a problem hiding this comment.
It appears that this change may have broken Scarecrows. A fix is at #431.
| export const getCrops = memoize( | ||
| (field, filterCondition) => | ||
| field.reduce((acc, row) => { | ||
| acc.push(...row.filter(filterCondition)) | ||
| /** | ||
| * @param {(?farmhand.plotContent)[][]} field | ||
| * @param {function(?farmhand.plotContent): boolean} condition | ||
| * @returns {?farmhand.plotContent} | ||
| */ | ||
| (field, condition) => { | ||
| const [plot = null] = | ||
| field.find(row => { | ||
| return row.find(condition) | ||
| }) ?? [] | ||
|
|
||
| return acc | ||
| }, []), |
There was a problem hiding this comment.
This wasn't being used, so I removed it.
| if (sanitizedState.field) { | ||
| // Update plot data | ||
| sanitizedState.field = sanitizedState.field.map(row => | ||
| row.map(plot => { | ||
| if (plot === null) { | ||
| return null | ||
| } | ||
|
|
||
| const { isFertilized, ...rest } = plot | ||
|
|
||
| return { | ||
| ...rest, | ||
|
|
||
| // Convert from isFertilized (boolean) to fertilizerType (enum) | ||
| fertilizerType: | ||
| rest.fertilizerType || | ||
| (isFertilized ? fertilizerType.STANDARD : fertilizerType.NONE), | ||
| } | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| const { tools: unlockedTools } = getLevelEntitlements( | ||
| levelAchieved(farmProductsSold(sanitizedState.itemsSold)) | ||
| ) | ||
|
|
||
| for (const tool of Object.keys(unlockedTools)) { | ||
| sanitizedState.toolLevels = unlockTool(sanitizedState.toolLevels, tool) | ||
| } | ||
|
|
||
| if ( | ||
| !sanitizedState.showHomeScreen && | ||
| sanitizedState.stageFocus === stageFocusType.HOME | ||
| ) { | ||
| sanitizedState.stageFocus = stageFocusType.SHOP | ||
| } | ||
|
|
||
| // TODO: Remove these cowInventory and cowForSale transformations after | ||
| // 3/15/2023 | ||
| sanitizedState.cowInventory = sanitizedState.cowInventory.map(cow => ({ | ||
| ownerId: id, | ||
| originalOwnerId: id, | ||
| timesTraded: 0, | ||
| ...cow, | ||
| })) | ||
|
|
||
| sanitizedState.cowForSale = { | ||
| ownerId: '', | ||
| originalOwnerId: '', | ||
| timesTraded: 0, | ||
| ...sanitizedState.cowForSale, | ||
| } | ||
|
|
There was a problem hiding this comment.
This function was long overdue for a cleanup, so I removed support for deprecated save files as part of this PR.
There was a problem hiding this comment.
This is pretty standard tsconfig.json boilerplate. Its presence is what causes the editor to recognize type errors as issues to present during development.
|
We discussed on Discord that the code looks good. I've QA'ed this thoroughly myself and there don't appear to be any regressions, so I'm going to merge this now. |
What this PR does
This PR begins Farmhand's adoption of Typed JavaScript. Typed JavaScript strikes a nice balance between TypeScript's full type safety and JavaScript's simplicity. The type system is defined entirely by JSDoc-style comments that are already pervasive throughout the codebase.
This transition is detailed in the ADR that is included in this PR. Additionally, this PR contains the following changes:
tsconfig.jsonfilesrc/utils/index.jsto be completely type-safeHow this change can be validated
A broad, light smoke test should be sufficient.
getPlotImageneeded the most substantive refactoring, so it would be worth giving the Field some extra attention to make sure it looks correct.Questions or concerns about this change
The adoption of Typed JavaScript will be a long road and may never be complete. I don't think this is a problem, as the code base and development experience benefits from each step towards complete type safety. The practical drawback with this change is that type errors can be visually annoying in development environments until they are corrected.
I'm personally motivated to update legacy code to be type safe as I go, but there is no expectation or requirement for anyone to do so as they work on the project. The type system is not integrated with the CI system for now, and that would not be a consideration until all type errors are fixed (if that ever happens). For now it's just another static analysis tool to aid in development that is roughly adjacent to ESLint and Prettier.
Additional information
This is how type errors present in Neovim and VSCode environments that are set up to support TypeScript:
To reiterate, these errors do not cause build failures. They are solely static analysis hints shown during development.