Chana/migrate analisis component to ts #56
Conversation
| @@ -58,7 +58,7 @@ export default function MonthsSlider({ | |||
| setMonthRange(range); | |||
| setDates({ min, max }); | |||
| }, | |||
| [], | |||
| [globalDates.min, setDates], | |||
There was a problem hiding this comment.
how is setDates here ? it's a setter not a variable.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
then why aren't we passing setMonthRange ?
There was a problem hiding this comment.
setMonthRange is stable by definition. The linter knows this since this PR from 2020. This component cannot know about the stability of its props.
| "plugins": [{ "name": "typescript-plugin-css-modules" }] | ||
| "plugins": [{ "name": "typescript-plugin-css-modules" }], | ||
| "strictNullChecks": true, | ||
| "noUncheckedIndexedAccess": true |
There was a problem hiding this comment.
needs better description of why you're enabeling this.
There was a problem hiding this comment.
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
| @@ -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) => ( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
then the right way is to put it in another commit explaining exactly what you wrote here.
@xaiki I can split the MonthsSlider commit to a new PR but I am having trouble seeing how my commit messages could be better. So, two questions:
|
|
what you've explained here is exactly what should go into the commit history. |
Setters, as all functions in javascript, are values and their pointers can change. 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.
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. That is why Object.keys was rewritten as Object.entries
juanigaray
force-pushed
the
chana/migrate-analisis-component-to-ts
branch
from
440dabc
to
b70aec1
Compare
| @@ -58,7 +58,7 @@ export default function MonthsSlider({ | |||
| setMonthRange(range); | |||
| setDates({ min, max }); | |||
| }, | |||
| [], | |||
| [globalDates.min, setDates], | |||
There was a problem hiding this comment.
then why aren't we passing setMonthRange ?
|
Conversations resolved off-review in group chat. |
No description provided.