Fail the build if we have type errors (and fix/ignore remaining errors) #1723

ob6160 · 2024-09-23T23:48:41Z

What does this change?

This rounds off the work I was doing a few years ago to migrate the project to Typescript
At the time, I got the build to a point where there were about 50+ unresolved type errors, meaning that we had to enable ignoreBuildErrors in Next to complete our migration from CRA
This PR ignores / refactors code where necessary in order to get our Typescript build passing, with the aim to give us a foundation to improve our types further over time

resolves #1366

vercel · 2024-09-23T23:48:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
toiletmap	✅ Ready (Inspect)	Visit Preview	Oct 6, 2024 7:46pm

ob6160 · 2024-09-24T00:10:56Z

next.config.js

-  typescript: {
-    // !! WARN !!
-    // Dangerously allow production builds to successfully complete even if
-    // your project has type errors.
-    // !! WARN !!
-    ignoreBuildErrors: true,
-  },


cypress · 2024-09-24T00:17:02Z

GBPTM Run #1338

Run Properties: Passed #1338 • 01b970835a ℹ️: Merge 79ea338a8cf063ab5c2048ed7335563f7311da04 into 6376c1f1bac919ad083124646c3e...

Project	`GBPTM`
Branch Review	`refs/pull/1723/merge`
Run status	`Passed #1338`
Run duration	`01m 48s`
Commit	`01b970835a ℹ️: Merge 79ea338a8cf063ab5c2048ed7335563f7311da04 into 6376c1f1bac919ad083124646c3e...`
Committer	`Oliver Barnwell`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`63`
View all changes introduced in this branch ↗︎

.eslintignore

src/api-client/withApollo.tsx

src/components/CodeViewer/CodeViewer.tsx

src/components/Feedback/Feedback.tsx

src/components/LooMap/LooMap.tsx

ob6160 · 2024-09-24T09:27:28Z

src/components/LooMap/LooMapLoader.tsx

  ssr: false,
 });

-const LooMap = (props: LooMapProps) => {
+const LooMap = (props?: Partial<LooMapProps>) => {


Not all LooMapProps are required

But are they all optional? Optional props run the risk of adding more confusion as they make things less explicit…

In this case they are, they’re optional on top of the defaults that we provide

ob6160 · 2024-09-24T09:29:28Z

tsconfig.json

+  "exclude": [
+    "node_modules",
+    "src/@types/resolvers-types.ts",
+    "scripts/areaToDatabase/*"


areaToDatabase is an archive of old js code which we will eventually re-write so we have the ability to populate our areas table from scratch again

so we exclude it for now as it won't pass the type checker

src/api/graphql/resolvers.ts

src/api-client/withApollo.tsx

src/components/LocationSearch/useNominatimAutocomplete.ts

src/components/LooMap/Markers.tsx

src/components/LooMap/ToiletMarkerIcon.tsx

src/components/LooMap/useLocateMapControl.ts

src/lib/openingTimes.ts

src/pages/explorer/search.page.tsx

add autogen files back to the linter ignore

mxdvl · 2024-10-07T06:04:26Z

src/components/LooMap/ToiletMarkerIcon.tsx

-const getIconAnchor = (dimensions: number[]) => [
-  dimensions[0] / 2,
-  dimensions[1],
+const ICON_DIMENSIONS: [number, number] = [22, 34] as const;


This is widened to the number double. For as const to work it must be paired with satisfies.

https://chrisvaillancourt.io/posts/combining-typescript-satisfies-and-const-assertion/

mxdvl · 2024-10-07T06:08:59Z

src/components/LooMap/useLocateMapControl.ts

-      if (this.options[option]) {
-        style = style + `${property}: ${this.options[option]};`;
+      const value = this.options[option as keyof LocationMarkerOptions];
+      if (value !== undefined) {


What if the value is numeric? Should we add a px suffix to make it a valid CSS length?

mxdvl · 2024-10-07T06:12:16Z

src/design-system/components/Button/Button.tsx

+
+  // Remove htmlElement from props
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  const { htmlElement, ...rest } = props;


Maybe this would make the linter happier? Underscores are understood to be unused variables…

Suggested change

const { htmlElement, ...rest } = props;

const { htmlElement: _, ...rest } = props;

ob6160 · 2024-10-07T08:56:49Z

Thanks for all the detailed comments @mxdvl — I'm going to merge this as-is and open follow-up PRs over time to continue to improve types & quality of migrated files in the project. I think just having our build step fail on type errors now will be super valuable and is something we've not turned on since we started the migration from js->ts back in '21-'22

vercel bot had a problem deploying to Preview September 23, 2024 23:49 Failure

ob6160 changed the title ~~Turn on strict typechecking~~ Fail the build if we have type errors (and fix/ignore remaining errors) Sep 24, 2024

vercel bot deployed to Preview September 24, 2024 00:02 View deployment

ob6160 force-pushed the ob/type-checking branch from eceaf30 to 53483ba Compare September 24, 2024 00:06

vercel bot deployed to Preview September 24, 2024 00:08 View deployment

ob6160 force-pushed the ob/type-checking branch from 53483ba to a671b34 Compare September 24, 2024 00:10

ob6160 commented Sep 24, 2024

View reviewed changes

ob6160 marked this pull request as ready for review September 24, 2024 00:12

vercel bot deployed to Preview September 24, 2024 00:12 View deployment

ob6160 force-pushed the ob/type-checking branch 3 times, most recently from 558b872 to 7c7fea8 Compare September 24, 2024 00:22

vercel bot deployed to Preview September 24, 2024 00:24 View deployment

ob6160 added the Chromatic starts the Chromatic GitHub action workflow label Sep 24, 2024

ob6160 requested review from mxdvl, marxian and Hoolean September 24, 2024 09:22

ob6160 commented Sep 24, 2024

View reviewed changes

.eslintignore Show resolved Hide resolved

ob6160 commented Sep 24, 2024

View reviewed changes

src/api-client/withApollo.tsx Outdated Show resolved Hide resolved

ob6160 commented Sep 24, 2024

View reviewed changes

src/api-client/withApollo.tsx Show resolved Hide resolved

ob6160 commented Sep 24, 2024

View reviewed changes

src/components/CodeViewer/CodeViewer.tsx Show resolved Hide resolved

ob6160 commented Sep 24, 2024

View reviewed changes

src/components/CodeViewer/CodeViewer.tsx Outdated Show resolved Hide resolved

ob6160 commented Sep 24, 2024

View reviewed changes

src/components/Feedback/Feedback.tsx Outdated Show resolved Hide resolved

ob6160 commented Sep 24, 2024

View reviewed changes

src/components/LooMap/LooMap.tsx Show resolved Hide resolved

ob6160 commented Sep 24, 2024

View reviewed changes

ob6160 force-pushed the ob/type-checking branch from 7c7fea8 to 0be8830 Compare September 25, 2024 15:43

vercel bot deployed to Preview September 25, 2024 15:45 View deployment

mxdvl reviewed Sep 25, 2024

View reviewed changes

src/api/graphql/resolvers.ts Show resolved Hide resolved

src/api-client/withApollo.tsx Show resolved Hide resolved

mxdvl reviewed Sep 25, 2024

View reviewed changes

ob6160 force-pushed the ob/type-checking branch from 0be8830 to d16a038 Compare September 25, 2024 23:04

vercel bot deployed to Preview September 25, 2024 23:06 View deployment

mxdvl reviewed Sep 26, 2024

View reviewed changes

ob6160 force-pushed the ob/type-checking branch from d16a038 to fe41cec Compare October 4, 2024 18:14

vercel bot deployed to Preview October 4, 2024 18:16 View deployment

vercel bot deployed to Preview October 6, 2024 15:33 View deployment

vercel bot deployed to Preview October 6, 2024 16:00 View deployment

vercel bot deployed to Preview October 6, 2024 16:07 View deployment

ob6160 requested a review from mxdvl October 6, 2024 16:10

ob6160 added 8 commits October 6, 2024 20:43

type all of the things

164677b

add autogen files back to the linter ignore

make getIcon private

6eaed9f

simplify places where we parseInt

616b64a

destructure width and height, use as const

58532c9

use URLSearchParams to simplify nominatim URL construction

df92e29

use 'satisfies'

ec76e5d

remove unnecessary argument

ba1d4c3

update types for terminatingLink

79ea338

ob6160 force-pushed the ob/type-checking branch from 0414463 to 79ea338 Compare October 6, 2024 19:43

vercel bot deployed to Preview October 6, 2024 19:46 View deployment

ob6160 added Chromatic starts the Chromatic GitHub action workflow and removed Chromatic starts the Chromatic GitHub action workflow labels Oct 6, 2024

mxdvl approved these changes Oct 7, 2024

View reviewed changes

ob6160 merged commit ecfa28a into main Oct 7, 2024
14 checks passed

ob6160 deleted the ob/type-checking branch October 7, 2024 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail the build if we have type errors (and fix/ignore remaining errors) #1723

Fail the build if we have type errors (and fix/ignore remaining errors) #1723

ob6160 commented Sep 23, 2024 •

edited

Loading

vercel bot commented Sep 23, 2024 •

edited

Loading

ob6160 Sep 24, 2024

cypress bot commented Sep 24, 2024 •

edited

Loading

ob6160 Sep 24, 2024

mxdvl Sep 25, 2024

ob6160 Sep 25, 2024

ob6160 Sep 24, 2024

ob6160 Sep 24, 2024

mxdvl Oct 7, 2024

mxdvl Oct 7, 2024

mxdvl Oct 7, 2024

ob6160 commented Oct 7, 2024 •

edited

Loading

	const { htmlElement, ...rest } = props;
	const { htmlElement: _, ...rest } = props;

Fail the build if we have type errors (and fix/ignore remaining errors) #1723

Fail the build if we have type errors (and fix/ignore remaining errors) #1723

Conversation

ob6160 commented Sep 23, 2024 • edited Loading

What does this change?

vercel bot commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

cypress bot commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob6160 commented Oct 7, 2024 • edited Loading

ob6160 commented Sep 23, 2024 •

edited

Loading

vercel bot commented Sep 23, 2024 •

edited

Loading

cypress bot commented Sep 24, 2024 •

edited

Loading

ob6160 commented Oct 7, 2024 •

edited

Loading