-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Reload dashboard when Stats API version changes #5362
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.
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
base: master
Are you sure you want to change the base?
Conversation
06e5394
to
d5a1f09
Compare
Idea from the lovely @apata for making easier breaking changes in the dashboard API: reload the dashboard when the internal API version changes.
d5a1f09
to
6d96a70
Compare
payload, | ||
EXPECTED_API_VERSION | ||
}) | ||
window.location.reload() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind, this risks us creating an endless reload loop for all dashboards when we fail to keep things in sync. There's a small chance of that happening, but the damage would be great.
I think we could mitigate it by instead doing window.location.replace
, adding the r=...
query parameter with value of EXPECTED_API_VERSION, like r=5
.
Then, in this function, we can make the reload conditional on query not containing r=${EXPECTED_API_VERSION}
.
There's a decent example of something similar in url-search-params.ts
: perhaps this logic could be colocated there?
@@ -115,7 +121,8 @@ defmodule PlausibleWeb.Api.StatsController do | |||
comparison_plot: comparison_result && plot_timeseries(comparison_result, metric), | |||
comparison_labels: comparison_result && label_timeseries(comparison_result, nil), | |||
present_index: present_index, | |||
full_intervals: full_intervals | |||
full_intervals: full_intervals, | |||
api_version: @api_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having it in the JSON payload is good: it catches the eye when refactoring the payloads server side. But, it also misses a few routes that the dashboard is using, like sites, segments, funnels.
Did you consider providing it in a header?
It seems to me that if it were a header, we could provide it with a router plug on all those APIs and use it as the definitive trigger for reloading incompatible dashboards.
Idea from the lovely @apata for making easier breaking changes in the dashboard API: reload the dashboard when the internal API version changes.
This will help us make 'breaking' changes easier.