feat: Allow date range selection in charts #3085
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
fhqvst
force-pushed
the
fhqvst/ui-187-allow-date-range-selection-in-charts
branch
from
b0122ad to
57d034c
Compare
fhqvst
force-pushed
the
fhqvst/ui-187-allow-date-range-selection-in-charts
branch
from
57d034c to
7af9a70
Compare
fhqvst
force-pushed
the
fhqvst/ui-187-allow-date-range-selection-in-charts
branch
from
7af9a70 to
65f2059
Compare
fhqvst
force-pushed
the
fhqvst/ui-187-allow-date-range-selection-in-charts
branch
from
65f2059 to
6f95944
Compare
fhqvst
force-pushed
the
fhqvst/ui-187-allow-date-range-selection-in-charts
branch
from
6f95944 to
cffaba0
Compare
| const { symbol, publisher, cluster, from, to, resolution } = parsed.data; | ||
|
|
||
| try { | ||
| checkMaxDataPointsInvariant(from, to, resolution); |
There was a problem hiding this comment.
Personally, I'm not a fan of functions with hidden side effects. I also think the implementation of checkMaxDataPointsInvariant is unnecessarily awkward -- I prefer to avoid the unnecessary use of let bindings, and it's less concise than it needs to be. Additionally, the current implementation is using exceptions as control flow which I generally try to avoid.
How something like this instead?
if ((to - from) / RESOLUTION_TO_SECONDS[resolution] > MAX_DATA_POINTS) {
return new Response("Unsupported resolution for date range", { status: 400 });
}
// ...
const ONE_MINUTE_IN_SECONDS = 60;
const ONE_HOUR_IN_SECONDS = 60 * ONE_MINUTE_IN_SECONDS;
const RESOLUTION_TO_SECONDS = {
"1 MINUTE": ONE_MINUTE_IN_SECONDS,
"5 MINUTE": 5 * ONE_MINUTE_IN_SECONDS,
"1 HOUR": ONE_HOUR_IN_SECONDS,
"1 DAY": 24 * ONE_HOUR_IN_SECONDS,
};There was a problem hiding this comment.
how would you feel about doing checkMaxDataPointsInvariant(...): boolean instead?
if (!checkMaxDataPointsInvariant()) {
return new Response("Unsupported resolution")
}
There was a problem hiding this comment.
I think it's reasonable to extract to a separate function but checkMaxDataPointsInvariant is IMO a pretty bad name, how about either getNumDataPoints(to, from, resolution) > MAX_DATA_POINTS or maybe isNumDataPointsOverMax(to, from, resolution)? I personally think getNumDataPoints is simpler / more obvious.
| const [resolution, setResolution] = useChartResolution(); | ||
|
|
||
| const handleLookbackChange = useCallback( | ||
| (newValue: Key) => { |
There was a problem hiding this comment.
I don't think there's any reason to use Key here -- if you make the resolutions an enum or a string union, the types in <Select> should work out that you can take the enum / union here and you don't have to type guard or cast
There was a problem hiding this comment.
iirc it did not wanna play nice with enums, but i'll have another look
i think the right thing to do is for Select to take a K generic (perhaps one that extend string | number)? maybe not
There was a problem hiding this comment.
hmm I think enums should be OK since we're using them elsewhere e.g. here, and here is an example of where we use it with a union in case you choose that.
I wouldn't use a string | number here; IMO a select should nearly always map to an enum or a union -- there are a restricted set of valid values and string | number implies a freeform input. Not only is the input not freeform so string | number doesn't accurately describe the values it produces, but also if that's what you use then you'll unnecessarily need to do validation & type checking OR type casting. Representing things as an enum/union is both more accurate and will type check correctly without additional checks.
| const chartRef = useRef<ChartRefContents | undefined>(undefined); | ||
| const isBackfilling = useRef(false); | ||
| const priceFormatter = usePriceFormatter(); | ||
| const resolutionRef = useRef(resolution); |
There was a problem hiding this comment.
What's the point of this ref and why not just use resolution directly everywhere?
There was a problem hiding this comment.
no point - i think we can delete this now
| }); | ||
|
|
||
| fetch(url, { signal: abortControllerRef.current.signal }) | ||
| .then(async (data) => historicalDataSchema.parse(await data.json())) |
There was a problem hiding this comment.
I don't think we should mix async/await and then, just move the schema parsing down to the next .then instead
| // Get the current historical price data | ||
| const currentHistoricalPriceData = chartRef.current.price | ||
| .data() | ||
| .filter((d) => isLineData(d)); |
There was a problem hiding this comment.
I'm not clear on why you need to filter these arrays, can you add a brief comment explaining why isLineData is necessary and what kind of data the data() function might return which wouldn't be line data?
There was a problem hiding this comment.
let me double check this, but i think price.data() returns an intersection of many types of Data (something like AreaData | LineData | ... which is why i needed that type guard
| }); | ||
|
|
||
| const confidenceHigh = chart.addSeries(AreaSeries, confidenceConfig); | ||
| const confidenceLow = chart.addSeries(AreaSeries, confidenceConfig); |
There was a problem hiding this comment.
Since you moved to using an AreaSeries and don't need to draw the two lines, could you compress this into a single series centered on the price instead of a series above and one below?
There was a problem hiding this comment.
i don't think we can do this if we want the purple background between the lines, but i can have another look
| export type Resolution = (typeof RESOLUTIONS)[number]; | ||
|
|
||
| export const LOOKBACKS = ["1m", "1H", "1D", "1W", "1M"] as const; | ||
| export type Lookback = (typeof LOOKBACKS)[number]; |
There was a problem hiding this comment.
I really don't like the name "lookbacks" for this -- if we do the auto-resolution adjust, this is more accurately a "snap to" than a lookback. Even now I think this is effectively behaving as a "snap to"
There was a problem hiding this comment.
yep yep, i don't have a strong opinion here. another candidate was INTERVAL, but i found that it could be easily confused with RESOLUTION. what do you think?
There was a problem hiding this comment.
the best I have in mind is SNAP_TO_PERIODS. I'm sure there's a better name, but INTERVAL doesn't seem clear to me either. Maybe @alexcambose has some ideas?
There was a problem hiding this comment.
QUICK_SELECT_WINDOWS maybe?
| queryParams.publisher = publisher; | ||
| } | ||
| if (cluster === "pythtest") { | ||
| queryParams.clusters = ["pythnet", "pythtest-conformance"]; |
There was a problem hiding this comment.
I'm confused why we would fetch from both clusters when pythtest is specified? It also does not look like the cluster is ever passed in so why even have this as a config?
There was a problem hiding this comment.
so this in an artifact from here, but for some reason i've messed it up (pythnet instead of pythtest)... and tbh i'm not even sure we wanna do this at all.
i'll remove the addition of pythtest-conformance so that cluster can only be pythnet or pythtest, and we can add back any conformance-related stuff later.
There was a problem hiding this comment.
Ah so to be clear pythtest is not a cluster name, we sometimes mix the names pythtest and pythtest-conformance but pythtest-conformance is the proper name.
I don't think you will ever have a good reason to display a price chart for the pythtest-conformance aggregate -- the aggregate price in pythtest-conformance doesn't actually mean anything since the publishers use that environment to test their feeds and so the data quality is wildly inaccurate. I'd probably just get rid of this. But if we find there is in fact some reason to bring this back, the clusters will be pythnet and pythtest-conformance, not pythtest which does not in fact correspond to a real cluster.
| AND publisher = '' | ||
| GROUP BY time | ||
| ORDER BY time ASC | ||
| PREWHERE |
There was a problem hiding this comment.
Hmm I'm not sure I understand the reason for splitting the where clause and using prewhere here, does postgres not optimize this query well?
There was a problem hiding this comment.
to really get to the bottom here we should probably measure a couple different queries and see which one is faster.
that said, the (cluster, publisher, symbol) combo follows the specified physical sort order of the table, and so forcing the engine to filter by that first should be some kind of guarantee that we're always doing lookups as fast as possible.
slight chance that this is a premature optimization, but it's so small i figured adding it couldnt hurt
There was a problem hiding this comment.
Yeah that's fine, it doesn't bother me to have it if you think it could help with perf (and has no risk of having the opposite impact by accident), but I also doubt this is going to do anything differently than what the query plan optimizer would do anyways...
Summary
Initially based on #2986, this PR adds support for "resolution" and "lookback" selection in the chart component. The chart will now also backfill more data in the correct resolution as the user is panning/zooming.
How has this been tested?
Todo
Future work