Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add typescript to codebase #178

Merged

Conversation

JamesLowenthal
Copy link
Contributor

@JamesLowenthal JamesLowenthal commented Mar 2, 2024

This PR adds typescript to the codebase. Reviewers see comments.

overrides: []
parserOptions:
ecmaVersion: latest
sourceType: module
rules:
no-unexpected-multiline: off
import/no-unresolved: off
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eslint doesn't seem to recognize some of parcels import patterns

data/score-cards.json Outdated Show resolved Hide resolved
scripts/base.ts Outdated Show resolved Hide resolved
"add-city": "node scripts/add-city.js",
"update-city-boundaries": "node scripts/update-city-boundaries.js",
"update-lots": "node scripts/update-lots.js",
"add-city": "ts-node-esm scripts/add-city.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to use ts-node-esm to run ts scripts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At work, we use node -r esbuild-register. You have to install esbuild-register in dev dependencies. I don't have a preference for either approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave it a quick try and had trouble with the loader? (apparently I needed to use the experimental loader to run these files). Didn't feel like digging into it so I'm sticking with ts-node-esm.

@JamesLowenthal JamesLowenthal marked this pull request as ready for review March 5, 2024 01:15
@JamesLowenthal JamesLowenthal mentioned this pull request Mar 9, 2024
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge improvement. Thank you!

@@ -22,7 +38,6 @@ const BASE_LAYERS = {
subdomains: "abcd",
minZoom: 0,
maxZoom: MAX_ZOOM,
ext: "png",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good catch TS.

Comment on lines +137 to +140
const loadParkingLot: (
cityId: CityId,
parkingLayer: GeoJSON<GeoJsonProperties, Geometry>
) => Promise<void> = async (cityId, parkingLayer) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit. This will probably need reformatting though

Suggested change
const loadParkingLot: (
cityId: CityId,
parkingLayer: GeoJSON<GeoJsonProperties, Geometry>
) => Promise<void> = async (cityId, parkingLayer) => {
const loadParkingLot = async (cityId: CityId, parkingLayer: GeoJSON<GeoJsonProperties, Geometry>): Promise<void> => {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto the other functions in this file like snapToCity

Comment on lines +277 to +279
} else {
throw new Error("#city-choice is not a select element");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: to avoid nesting like this, you can do an early return. Invert the conditional to check if this error condition is true and throw an exception. Then you can avoid indenting the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest I'm not sure how crazy I am about early return in this instance. In this case I feel the indentation makes it clearer that the cityToggleElement has changed type within the scope of the curly braces by narrowing. That said I'm in favor of resolving discussions about style by favoring consistency with the style of the rest of the organization...so if early return is the preference I'm happy to make that change, I just wanted to state my thought process for the record.

src/js/share.ts Outdated
/**
* Copy `value` to the user's clipboard
*
* @param string value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get read of these types of docstring values. We were using them as a precursor to TypeScript. Now they're redundant 🎉

Ditto other places in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got rid of them and the comments. Not sure if you'd agree with that but I feel like both are redundant at this point.

src/js/share.ts Outdated Show resolved Hide resolved
// We put the event listener on `map` because it is never erased, unlike the copy button
// being recreated every time the score card changes. This is called "event delegation".
const mapElement: HTMLElement | null = document.querySelector("#map");
if (mapElement) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can invert the conditional. If !mapElement, then return. It avoids this indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the other comment. I like that this makes it explicit that mapElement within the braces is no longer HTMLElement | null but rather is HTMLElement after narrowing the type via the conditional.


export type CityId = string;

export interface ScoreCardDetails {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

tsconfig.json Outdated
"exactOptionalPropertyTypes": true /* Interpret optional property types as written, rather than adding 'undefined'. */,
"noImplicitReturns": true /* Enable error reporting for codepaths that do not explicitly return in a function. */,
"noFallthroughCasesInSwitch": true /* Enable error reporting for fallthrough cases in switch statements. */,
"skipLibCheck": true /* Skip type checking all .d.ts files. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems not, good catch, not sure how that one snuck in.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This is great!

Re: early return, not a big deal either way. I'm a huge fan of the technique because it allows you to deal with edge cases right up-front, then stop having to ever think about them in the rest of the body. For example, in that particular commented code, you deal with the edge case of null up-front, then no longer have to think about it. Compared to the if-else making you remember "wait what was the conditional again?" throughout the entire function body since it's all nested. https://dev.to/bespoyasov/refactoring-tools-early-return-for-flatter-conditions-d1p talks about this idea. We generally do indeed use early returns, but it's not a big deal and I'm fine with merging as is if you prefer.

Thanks again for this change 🙌

@Eric-Arellano Eric-Arellano merged commit 096b586 into ParkingReformNetwork:main Apr 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants