-
Notifications
You must be signed in to change notification settings - Fork 396
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
convert Home page to use TypeScript #3124
Conversation
src/components/Home/Home.tsx
Outdated
const Home = ({ strings }) => ( | ||
export interface HomePageProps { | ||
user?: string | ||
strings: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be tedious to type all of the strings separately. . . we could just type it as the en-us JSON file type dict, or even just generic string dict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like having a separate file only for types and importing what's needed? can you give an example please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The easy solution is just to type strings as {[key: string]: string} which you can share across everything that's using the strings.
Alternatively, since the en-US.json file has all the keys in it, it should be possible to tell TS to construct a type out of that. That would be nice because you'll get autocomplete/checking if there's a typo, but I'm not 100% sure how to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so looks like there are 3 approaches to this:
- just use a generic string dictionary (updated the PR to use this!)
- use JSON file for type inference. this requires installing a JSON schema generator such as https://github.com/vega/ts-json-schema-generator. (I tried it but it didn't work because it only supports '.ts', '.tsx', '.d.ts', '.cts', '.d.cts', '.mts', '.d.mts' formats.)
- create a separate interface for strings like so:
interface Strings {
app_name: string;
app_description: string;
home_background_by: string;
// ...
}
interface HomePageProps {
user?: string;
strings: Strings;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@howardchung let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe since the JSON files will be flat we can use keyof to generate a union of the possible key values instead of string?
https://www.typescriptlang.org/docs/handbook/2/keyof-types.html
But we can do that in a separate change
<StyledDiv> | ||
<div className="headline">{strings.home_sponsored_by}</div> | ||
<div className="images"> | ||
{config.VITE_ENABLE_DOTA_COACH && ( | ||
{config.VITE_ENABLE_DOTACOACH && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also fixes a typo here.
As part of a major refactoring (reference), the app will gradually be using TypeScript and this is the initial PR for the Home page to be converted to TS.