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

convert Home page to use TypeScript #3124

Merged
merged 2 commits into from
Nov 20, 2023
Merged

Conversation

mehrdadrafiee
Copy link
Contributor

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.

const Home = ({ strings }) => (
export interface HomePageProps {
user?: string
strings: {
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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;
}

Copy link
Contributor Author

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.

Copy link
Member

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 && (
Copy link
Contributor Author

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.

@howardchung howardchung merged commit 6d3313d into odota:master Nov 20, 2023
4 checks passed
This pull request was closed.
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