Add measurements types #1910
Merged
Add measurements types #1910
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nextstrain-bot
temporarily deployed
to
auspice-measurements-ty-moos3q
joverlee521
force-pushed
the
measurements-types
branch
from
f2fe21d to
891488a
Compare
genehack
approved these changes
Nov 19, 2024
victorlin
reviewed
Nov 19, 2024
victorlin
reviewed
Nov 19, 2024
victorlin
reviewed
Nov 21, 2024
joverlee521
added a commit
that referenced
this pull request
Nov 22, 2024
Because the query params are always parsed as strings, the measurements URL query `mf_*` will fail if the measurements' raw data are not strings. Instead of doing string transformations throughout the code, just convert all measurements metadata to strings during parsing of the JSON. I've left the measurement `value` field as a number because converting it to string resulted in a lot of calculation errors in D3. I will revisit this in the future when I add types to the measurementsD3 file. Renamed `MeasurementMetadata` to `JsonMeasurementMetadata` to distinguish from `Measurement.[key: string]` as suggested by @victorlin in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 22, 2024
Add explicit return types for various `map` as suggested in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 22, 2024
Specifying the values again make them more readable as suggested in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 22, 2024
As suggested in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 22, 2024
Removes the redundant `dataType` param and just does type checking directly on the `data` param as discussed in review <#1910 (comment)>
joverlee521
force-pushed
the
measurements-types
branch
from
891488a to
adac6db
Compare
joverlee521
added a commit
that referenced
this pull request
Nov 22, 2024
As suggested in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 22, 2024
Removes the redundant `dataType` param and just does type checking directly on the `data` param as discussed in review <#1910 (comment)>
joverlee521
force-pushed
the
measurements-types
branch
from
adac6db to
435b3f8
Compare
joverlee521
added a commit
that referenced
this pull request
Nov 23, 2024
Because the query params are always parsed as strings, the measurements URL query `mf_*` will fail if the measurements' raw data are not strings. Instead of doing string transformations throughout the code, just convert all measurements metadata to strings during parsing of the JSON. I've left the measurement `value` field as a number because converting it to string resulted in a lot of calculation errors in D3. I will revisit this in the future when I add types to the measurementsD3 file. Renamed `MeasurementMetadata` to `JsonMeasurementMetadata` to distinguish from `Measurement.[key: string]` as suggested by @victorlin in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 23, 2024
Add explicit return types for various `map` as suggested in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 23, 2024
Specifying the values again make them more readable as suggested in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 23, 2024
As suggested in review <#1910 (comment)>
joverlee521
added a commit
that referenced
this pull request
Nov 23, 2024
Removes the redundant `dataType` param and just does type checking directly on the `data` param as discussed in review <#1910 (comment)>
joverlee521
force-pushed
the
measurements-types
branch
from
435b3f8 to
5967b37
Compare
3 tasks
genehack
approved these changes
Nov 25, 2024
Moving in preparation for using this type for the measurements thunk functions as well. I had considered adding a new `src/actions/types.ts` file, but that will be confusing with the action types in `src/actions/types.js`.
Add types for most measurements specific files. I'm delaying converting the measurementsD3.js file because the import of `event` from d3-selection causes an error. This is likely due to the d3 types not matching the version of d3-selection.¹ I decided to place the measurements JSON types in the same file as the measurements state types so that it's easy to compare the data structures before and after parsing. There are many type errors that will be fixed in subsequent commits. ¹ <#1900>
Instead of using empty placeholders, be explicit with `undefined` to show lack of collections data.
Reflect reality where we only expect to change the measurement collection after all of the measurement data have been properly `loaded`.
Use of `lodash.pick` causes type error ``` src/actions/measurements.ts(211,3): error TS2322: Type 'Partial<ControlsState>' is not assignable to type 'MeasurementsControlState'. Property 'measurementsGroupBy' is optional in type 'Partial<ControlsState>' but required in type 'MeasurementsControlState'. ``` Instead of using type casting, just replace `lodash.pick` for the proper type assignment.
Fix many type errors that resulted from mismatched types in `JsonCollection` and `Collection`.
Resolves type error when using a plain object to keep counts of grouping values: ``` src/actions/measurements.ts(292,56): error TS2538: Type 'false' cannot be used as an index type. ```
Update the `Query` type to reflect the returned type of `querystring.parse`.¹ This led me to realize that there's a currently a bug in the measurement filter query param matching when filtering using values that are _not_ a string the raw measurement metadata. This will be fixed in the following commit. ¹ <https://nodejs.org/docs/latest-v22.x/api/querystring.html#querystringparsestr-sep-eq-options>
Because the query params are always parsed as strings, the measurements URL query `mf_*` will fail if the measurements' raw data are not strings. Instead of doing string transformations throughout the code, just convert all measurements metadata to strings during parsing of the JSON. I've left the measurement `value` field as a number because converting it to string resulted in a lot of calculation errors in D3. I will revisit this in the future when I add types to the measurementsD3 file. Renamed `MeasurementMetadata` to `JsonMeasurementMetadata` to distinguish from `Measurement.[key: string]` as suggested by @victorlin in review <#1910 (comment)>
Add explicit return types for various `map` as suggested in review <#1910 (comment)>
Specifying the values again make them more readable as suggested in review <#1910 (comment)>
As suggested in review <#1910 (comment)>
Removes the redundant `dataType` param and just does type checking directly on the `data` param as discussed in review <#1910 (comment)>
joverlee521
force-pushed
the
measurements-types
branch
from
5967b37 to
6dfb02a
Compare
I've tested with the test narrative with measurements and the test app for auspice.us and didn't see any bugs. I'll plan to merge this on Monday as I want to build the color tree by measurements feature on top of these changes. |
victorlin
approved these changes
Dec 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
test narrative with measurements
Description of proposed changes
Add types for most measurements specific files. See commits for details.
I did not run
strictNullChecks, I'll plan to revisit them later.Related issue(s)
Motivated by #1819 (comment).
Checklist