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.

Sign up for GitHub

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

Chana/migrate analisis component to ts #56

Merged
merged 4 commits into from
Feb 1, 2024
Merged

Chana/migrate analisis component to ts #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5182f1c
add missing dependency to MonthsSlider hook
Jan 29, 2024
bac0243
add missing checks to tsconfig
Jan 29, 2024
b70aec1
migrate analisis component to ts
Jan 29, 2024
a7a8850
add --noEmit option to build to make use of Vite ts support
Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"dev": "vite",
"predeploy": "npm run build",
"deploy": "gh-pages -d dist",
"build": "node services/google-sheets.js > public/data/casos.json && tsc && vite build",
"build": "node services/google-sheets.js > public/data/casos.json && tsc --noEmit && vite build",
"lint": "eslint . --ext js,jsx --report-unused-disable-directives --max-warnings 0",
"preview": "vite preview"
},
Expand Down
2 changes: 1 addition & 1 deletion src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { Markers } from "./components/Markers.jsx";
import Main2 from "./components/Main2.jsx";
import Popup from "./components/Popup.jsx";
import Filtros from "./components/Filtros.jsx"; // Cambia la ruta a tu formulario
import Analisis from "./components/Analisis.jsx";
import Analisis from "./components/Analisis.tsx";

import mystyle from "./mystyle.json";
import MonthsSlider from "./components/MonthsSlider.tsx";
Expand Down
42 changes: 24 additions & 18 deletions src/components/Analisis.jsx → src/components/Analisis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,26 @@
import styles from "./Analisis.module.css";
import { Link as ScrollLink } from "react-scroll";
import PropTypes from "prop-types";

export default function Analisis({ min, max, total, tipos, componentes }) {
type PropWithByName = {
// TODO: These strings are constant and could be narrowed down to a union
byName: Record<string, number[]>;
};

interface Props {
min: Date;
max: Date;
total: number;
tipos: PropWithByName;
componentes: PropWithByName;
}

export default function Analisis({
min,
max,
total,
tipos,
componentes,
}: Props) {
return (
/* TODO: extract this ID, "analisis", which is also used in Main2 and App, to a constant */
<div id="analisis" className={styles.analisis}>
Expand All @@ -27,17 +45,17 @@ export default function Analisis({ min, max, total, tipos, componentes }) {
</div>

<div className={styles.analisisDatos}>
{Object.keys(tipos.byName).map((t, i) => (
{Object.entries(tipos.byName).map(([t, value], i) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a functional change and should go to it's own commit.
as a rule you should refrain to 'better the code' while migrating.

Copy link
Collaborator Author

@juanigaray juanigaray Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll keep it in mind in the future, but I believe this change is part of the migration. This does not change any functionality and fixes problems with Typescript.

You see, when you access tipos.byName[t] you don't have a guarantee that that doesn't evaluate to undefined in Typescript. That's because byName isn't a Record<NarrowType, Whatever> but a Record<string, string>, which makes it possible to access byName["someString"] and get undefined.

If I hadn't changed this, I would have had typing errors. In my opinion this change is pertinent to the commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

then the right way is to put it in another commit explaining exactly what you wrote here.

<div className={styles.datos1} key={i}>
<h1 className={styles.datoN1}>{tipos.byName[t].length}</h1>
<h1 className={styles.datoN1}>{value.length}</h1>
<div>
<p className={styles.textAnalisis}>{t}</p>
</div>
</div>
))}
{Object.keys(componentes.byName).map((t, i) => (
{Object.entries(componentes.byName).map(([t, value], i) => (
<div className={styles.datos2} key={i}>
<h1 className={styles.datoN2}>{componentes.byName[t].length}</h1>
<h1 className={styles.datoN2}>{value.length}</h1>
<div>
<p className={styles.textAnalisis}>{t}</p>
</div>
Expand Down Expand Up @@ -80,15 +98,3 @@ export default function Analisis({ min, max, total, tipos, componentes }) {
</div>
);
}

const PropWithByName = PropTypes.shape({
byName: PropTypes.array.isRequired,
}).isRequired;

Analisis.propTypes = {
min: PropTypes.instanceOf(Date),
max: PropTypes.instanceOf(Date),
total: PropTypes.number.isRequired,
tipos: PropWithByName,
componentes: PropWithByName,
};
2 changes: 1 addition & 1 deletion src/components/MonthsSlider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export default function MonthsSlider({
setMonthRange(range);
setDates({ min, max });
},
[],
[globalDates.min, setDates],
Copy link
Contributor

Choose a reason for hiding this comment

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

how is setDates here ? it's a setter not a variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Setters, as all functions in javascript, are values and their pointers can change. This is why we use useCallback, for instance. If this component were to receive a different setter, it would need to change its callback.

also, this was raising a warning from the linter, and I believe we should address linter warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

then why aren't we passing setMonthRange ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setMonthRange is stable by definition. The linter knows this since this PR from 2020. This component cannot know about the stability of its props.

);

return (
Expand Down
14 changes: 13 additions & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,19 @@
"isolatedModules": true,
"noEmit": true,
"jsx": "react-jsx",
"plugins": [{ "name": "typescript-plugin-css-modules" }]
"plugins": [{ "name": "typescript-plugin-css-modules" }],
// https://www.typescriptlang.org/tsconfig#strictNullChecks
"strictNullChecks": true,
/* Indexed access of an Array<T> can return T, but if the index is out of bounds, it returns undefined.
* This option makes the indexed access of Array<T> to evaluate to T | undefined.
*
* const numbers: Array = [];
* // this should fail, type of test should be number | undefined
* const test = numbers[0].toFixed();
*
*/
// https://www.typescriptlang.org/tsconfig#noUncheckedIndexedAccess
"noUncheckedIndexedAccess": true
Copy link
Contributor

Choose a reason for hiding this comment

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

needs better description of why you're enabeling this.

Copy link
Collaborator Author

@juanigaray juanigaray Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could the documentation do? I could just link https://www.typescriptlang.org/tsconfig#noUncheckedIndexedAccess. Otherwise I can provide an explanation for why accessing myArray[n], with n being any number, can return undefined and should be typed as such

Copy link
Contributor

Choose a reason for hiding this comment

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

both =)

},
"include": ["src"],
"references": [{ "path": "./tsconfig.node.json" }]
Expand Down
Loading