ref(pageFilters): Constrict types on updateParams better #31073
Conversation
| project: project?.map(String), | ||
| environment, | ||
| start: statsPeriod ? null : start instanceof Date ? getUtcDateString(start) : start, | ||
| end: statsPeriod ? null : end instanceof Date ? getUtcDateString(end) : end, |
There was a problem hiding this comment.
@evanpurkhiser We should probably check if the dates here are valid, the instanceof new Date("invalid date") will still pass here otherwise. UX wise, I'm not sure if it's best to discard invalid dates or throw an error. I'm in favour of throwing so that in case this happens we have visibility and can fix it.
There was a problem hiding this comment.
We probably should check somewhere else if the dates are valid or not.
This code is a bit of a rats nest, before it wasn't even handling string-ification of date objects afaict.
For now I am OK with leaving this as it is, since it's at least a big improvement over how it worked before 😬
There was a problem hiding this comment.
Makes sense - I'm lacking context on some of these components and how they all work together in the greater deal of things, so I may say something dumb every now and then 😅 Def a nice improvement from before though 🙏🏼
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-pagefilters-constrict-types-on-updateparams-better
branch
from
76b8af9
to
088f128
Compare
This narrows the types on the updateParams function call to better reflect what is actually going on - The UrlParams type has been renamed to `UpdateParams` and `PageFilterQuery`, which better reflect how the parameters are transformed when being shuttled between query parameters and more structured 'state' objects. This is especially important when passing these parameters to the `setPageFiltersStorage` function which now specifically always wants the `project` parameters to be a string array. - Add a test case to cover the regression that occurred in 0bbbfab, where we could not call `.map` on a string, which typescript failed to catch since the types in the pageFilters actionCreators were too wide and in some cases incorrect. This test would have failed in 0bbbfab.
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-pagefilters-constrict-types-on-updateparams-better
branch
from
088f128
to
1bb3d6d
Compare
evanpurkhiser
deleted the
evanpurkhiser/ref-pagefilters-constrict-types-on-updateparams-better
branch
This narrows the types on the updateParams function call to better reflect what is actually going on
The UrlParams type has been renamed to
UpdateParamsandPageFilterQuery, which better reflect how the parameters are transformed when being shuttled between query parameters and more structured 'state' objects.This is especially important when passing these parameters to the
setPageFiltersStoragefunction which now specifically always wants theprojectparameters to be a string array.Add a test case to cover the regression that occurred in 0bbbfab, where we could not call
.mapon a string, which typescript failed to catch since the types in the pageFilters actionCreators were too wide and in some cases incorrect.This test would have failed in 0bbbfab.