Add Term Type #361

Merged
j285he merged 11 commits from j285he-add-term-type into main 1 year ago
j285he commented 1 year ago
Collaborator
There is no content yet.
j285he added 2 commits 1 year ago
e5fd9bfec9 Add Term type
131a667ddf TermEvent->TermPage for consistency
j285he added 1 commit 1 year ago
691b749bb7 Add type to getAllEvents
Poster
Collaborator

I think problem with generate-calendar not working is that it imports getAllEvents which is in the file lib/events.ts that imports the new type from @/utils. Since generate-calendar.ts is a ts-node file the paths/aliases in tsconfig don't work (https://github.com/TypeStrong/ts-node/issues/138)

Some solutions may be

Or may be I'm missing something. What do you think?

I think problem with `generate-calendar` not working is that it imports `getAllEvents` which is in the file `lib/events.ts` that imports the new type from `@/utils`. Since `generate-calendar.ts` is a `ts-node` file the paths/aliases in `tsconfig` don't work (https://github.com/TypeStrong/ts-node/issues/138) Some solutions may be * use a library like this https://www.npmjs.com/package/tsconfig-paths * in `events.ts`, use `../utils` instead of `@/utils` -- we could do this for all lib files to make things consistent Or may be I'm missing something. What do you think?
j285he requested review from n3parikh 1 year ago
j285he requested review from a258wang 1 year ago
j285he added 1 commit 1 year ago
fd5c244f98 Change alias for utils
a258wang reviewed 1 year ago
export const TERMS = ["winter", "spring", "fall"] as const;
export type Term = typeof TERMS[number];
Owner

As a noob, I'm curious: what's the advantage to defining the Term type this way, instead of using a union of strings ("winter" | "spring" | "fall")?

As a noob, I'm curious: what's the advantage to defining the `Term` type this way, instead of using a union of strings (`"winter" | "spring" | "fall"`)?
Poster
Collaborator

I saw other files (lib/events and lib/news) specifically defining TERMS = ["winter", "spring", "fall"] and I thought it would be useful to move that into utils. More generally, if we were to change what TERMS is, say if "spring" was renamed to "summer", then we would only need one code change here (although that's probably never going to happen).

Term = typeof TERMS[number] is the syntax to transform the const array to type "winter" | "spring" | "fall".

I saw other files (`lib/events` and `lib/news`) specifically defining `TERMS = ["winter", "spring", "fall"]` and I thought it would be useful to move that into `utils`. More generally, if we were to change what `TERMS` is, say if "spring" was renamed to "summer", then we would only need one code change here (although that's probably never going to happen). `Term = typeof TERMS[number]` is the syntax to transform the const array to type `"winter" | "spring" | "fall"`.
a258wang reviewed 1 year ago
utils.ts Outdated
// https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
export function isTerm(x: string): x is Term {
return x === "winter" || x === "spring" || x === "fall";
Owner

Is there a reason for this approach instead of return TERMS.includes(x);?

Is there a reason for this approach instead of `return TERMS.includes(x);`?
Poster
Collaborator

Because I used as const on TERMS, I believe TypeScript infers the "narrowest type", so TypeScript identifies TERMS as a readonly array of ["winter", "spring", "fall"] instead of an array of strings. Therefore using return TERMS.includes(x) gives me the error Argument of type 'string' is not assignable to parameter of type '"winter" | "spring" | "fall"'.

I think the reasoning for using as const to get the "narrowest type" is so that we can define the type Term using the array TERMS. If as const was not present then Term would just be type string[] (which is not specific enough for our purposes).

Because I used `as const` on `TERMS`, I believe TypeScript infers the "narrowest type", so TypeScript identifies `TERMS` as a readonly array of `["winter", "spring", "fall"]` instead of an array of strings. Therefore using `return TERMS.includes(x)` gives me the error `Argument of type 'string' is not assignable to parameter of type '"winter" | "spring" | "fall"'`. I think the reasoning for using `as const` to get the "narrowest type" is so that we can define the type `Term` using the array `TERMS`. If `as const` was not present then `Term` would just be type `string[]` (which is not specific enough for our purposes).
Poster
Collaborator

Let me know if there's a way to improve this. I agree it does look clunky

Let me know if there's a way to improve this. I agree it does look clunky
Owner

Ahh okay that's really interesting, thanks for the explanation!

Personally, I think the way this is written right now is actually quite readable. My only concern was if we ever wanted to, say, change "spring" to "summer", then we would have to make the change in two separate places (albeit two places in the same file).

I just did some experimenting and it seems like return TERMS.some(term => x === term); could work - this solves the "change in two separate places" issue, but it's also a tad less readable in my opinion. I'll leave it up to you to decide what to use!

Ahh okay that's really interesting, thanks for the explanation! Personally, I think the way this is written right now is actually quite readable. My only concern was if we ever wanted to, say, change "spring" to "summer", then we would have to make the change in two separate places (albeit two places in the same file). I just did some experimenting and it seems like `return TERMS.some(term => x === term);` could work - this solves the "change in two separate places" issue, but it's also a tad less readable in my opinion. I'll leave it up to you to decide what to use!
Poster
Collaborator

Good point and good idea. Wonder if anyone else has thoughts or opinions on this.

Good point and good idea. Wonder if anyone else has thoughts or opinions on this.
Poster
Collaborator

I liked the implementation and changed it. Let me know if anyone wants it reversed.

I liked the implementation and changed it. Let me know if anyone wants it reversed.
a258wang approved these changes 1 year ago
j285he added 2 commits 1 year ago
j285he added 1 commit 1 year ago
92a115fc35 Use TERMS.some
j285he added 1 commit 1 year ago
5e925e3e59 Fix members type error
j285he added 1 commit 1 year ago
j285he added 1 commit 1 year ago
5a87174b13 Merge branch 'main' into j285he-add-term-type
j285he added 1 commit 1 year ago
95287b4de7 Fix type mistake (?)
Poster
Collaborator

Our CI does highlight type errors with the new custom Term type, see 5a87174b13 vs 95287b4de7.

Our CI *does* highlight type errors with the new custom Term type, see 5a87174b13bcaa6857c43c0a79cfe7a87a55d4f8 vs 95287b4de7aa4e0e3175d8865015efc4ff83d977.
n3parikh approved these changes 1 year ago
n3parikh left a comment
Owner

Looks great, thanks for this!

Sorry for the very slow review

Looks great, thanks for this! Sorry for the very slow review
): Promise<Member[]> {
if (term !== "winter" && term !== "spring" && term !== "fall") {
throw new Error(`[getMembers] Not a valid term: "${term}"`);
}
Owner

Nice!

Nice!
j285he merged commit 2264e60852 into main 1 year ago
j285he referenced this issue from a commit 1 year ago
j285he deleted branch j285he-add-term-type 1 year ago

Reviewers

a258wang approved these changes 1 year ago
n3parikh approved these changes 1 year ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 2264e60852.
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#361
Loading…
There is no content yet.