-
Notifications
You must be signed in to change notification settings - Fork 11
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
RAD-5740: cache-recent-locations-in-websdk-with-variable-ttl #86
RAD-5740: cache-recent-locations-in-websdk-with-variable-ttl #86
Conversation
Ask about which branch to merge this into |
You can get prompted like after which I believe is intended |
Note time to live specifically exists for |
src/index.js
Outdated
if (!publishableKey) { | ||
console.error('Radar "initialize" was called without a publishable key'); | ||
} | ||
Storage.setItem(Storage.PUBLISHABLE_KEY, publishableKey); | ||
if(options){ |
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.
this will technically always be set since we have the default argument above
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.
👍
src/index.js
Outdated
if (!publishableKey) { | ||
console.error('Radar "initialize" was called without a publishable key'); | ||
} | ||
Storage.setItem(Storage.PUBLISHABLE_KEY, publishableKey); | ||
if(options){ | ||
let {locationTimeToLive} = options |
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.
use const
instead of let
if it's not being reassigned.
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.
👍
src/index.js
Outdated
if(options){ | ||
let {locationTimeToLive} = options | ||
|
||
if(locationTimeToLive && typeof(locationTimeToLive) === 'number'){ |
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.
Maybe we should throw a warning if it's set but not a number, but also be more lenient and accept number strings.
if (locationTimeToLive) {
const number = Number(locationTimeToLive);
if (Number.isNaN(number)) {
console.warn('Radar SDK: invalid number for option "locationTimeToLive"'));
} else {
Storage.setItem(Storage.LOCATION_TIME_TO_LIVE, locationTimeToLive)
}
}
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.
👍
src/navigator.js
Outdated
Storage.setItem(Storage.LAST_LOCATION_TIME, Date.now()) | ||
Storage.setItem(Storage.LATITUDE, latitude) | ||
Storage.setItem(Storage.LONGITUDE, longitude) | ||
Storage.setItem(Storage.ACCURACY, accuracy) |
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.
I think we can just roll these all up into JSON and use one key instead of having the separate fields. Also maybe we should only cache it if the setting is enabled.
if (locationTimeToLive) {
const lastLocation = { latitude, longitude, accuracy, updatedAt: Date.now() };
Storage.setItem(Storage.LAST_LOCATION, JSON.stringify(lastLocation));
}
And then when we read it out, we can do JSON.parse
if (locationTimeToLive) {
try {
const lastLocation = JSON.parse(Storage.getItem(Storage.LAST_LOCATION));
const { latitude, longitude, accuracy, updatedAt } = lastLocation;
// check expiration stuff goes here
} catch (e) {
console.warn('Radar SDK: could not load cached location.');
}
}
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.
👍
Okay changes made. Back at you @kochis |
src/index.js
Outdated
if (!publishableKey) { | ||
console.error('Radar "initialize" was called without a publishable key'); | ||
} | ||
Storage.setItem(Storage.PUBLISHABLE_KEY, publishableKey); | ||
|
||
if(Object.keys(options).length === 0){ | ||
const {locationTimeToLive} = options |
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.
Lets actually name this cacheLocationMinutes
. It's a bit more clear what it's used for (and the format it expects).
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.
👍
src/index.js
Outdated
if (!publishableKey) { | ||
console.error('Radar "initialize" was called without a publishable key'); | ||
} | ||
Storage.setItem(Storage.PUBLISHABLE_KEY, publishableKey); | ||
|
||
if(Object.keys(options).length === 0){ |
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.
Is this check correct? I also don't think we need it (options will be {}
by default).
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.
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.
Wait I see what you mean about it being 0. Which is really weird cause when I was testing it yesterday the code worked, but maybe it was just using old cache values and I didn't realize. Also yeah that's a good point that this is unneeded
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.
I more so did it in case we have options in the future to avoid checking all the options upon first loading if there were no objects passed in by default, but right now since we have only one it is fine
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.
👍
src/navigator.js
Outdated
@@ -8,6 +9,24 @@ class Navigator { | |||
return reject(STATUS.ERROR_LOCATION); | |||
} | |||
|
|||
let locationTimeToLive = parseFloat(Storage.getItem(Storage.LOCATION_TIME_TO_LIVE)); |
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.
const
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.
👍
src/navigator.js
Outdated
if (locationTimeToLive) { | ||
const lastLocation = { latitude, longitude, accuracy, updatedAt: Date.now() }; | ||
Storage.setItem(Storage.LAST_LOCATION, JSON.stringify(lastLocation)); | ||
} |
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.
It probably makes sense to bake in the expiration here too.
if (locationTimeToLive) {
const updatedAt = Date.now();
const expiresAt = updatedAt + (locationTimeToLive * 60 * 1000); // convert to ms
const lastLocation = { latitude, longitude, accuracy, updatedAt, expiresAt };
Storage.setItem(Storage.LAST_LOCATION, JSON.stringify(lastLocation));
}
Then when we read it out, we just need to check
if (Date.now() < parseInt(lastLocation.expiresAt())) {
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.
Yeah sure but then what's the point of storing updatedAt? Like either way you only have to store one or the other? I just happened to choose updatedAt vs expiresAt
src/index.js
Outdated
if (!publishableKey) { | ||
console.error('Radar "initialize" was called without a publishable key'); | ||
} | ||
Storage.setItem(Storage.PUBLISHABLE_KEY, publishableKey); | ||
|
||
|
||
const {cacheLocationMinutes} = options |
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.
nit: space around the brackets { cacheLocationMinutes }
for consistency, and semicolon at end
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.
👍
|
||
const {cacheLocationMinutes} = options | ||
|
||
if (cacheLocationMinutes) { |
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.
hm we might want an else
that clears it if cachedLocationMinutes
is not passed in. in the past, I've run into issues from local storage items getting set and sticking around unintended
for example if you call initialize
with cachedLocationMinutes=5, then you want to turn it off or on a separate initialize it will stick around even if you don't pass in the field.
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.
👍
const cacheLocationMinutes = parseFloat(Storage.getItem(Storage.CACHE_LOCATION_MINUTES)); | ||
if (cacheLocationMinutes) { | ||
try { | ||
const lastLocation = JSON.parse(Storage.getItem(Storage.LAST_LOCATION)); |
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.
we should probably skip the rest if LAST_LOCATION is null, think it will error for the first call and go to the warn since there will be undefined fields
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.
👍
Back at you @corypisano |
cool i'll bump the sdk version, and just gonna fix some formatting (we should bring lint setup here but separate issue) |
Requirements
Difficult to write test for. Tested manually:
QA details
Include screenshots, responses, or steps to verify the code works as expected
Can verify locally by setting up local sdk, or verify that it works using vercel:
https://frontend-git-boon-sdk-demo-branch-radarlabs.vercel.app/demo/js
Currently cached location time is 30 seconds. So go to link in safari, enter prompt for location.
After you can continuously reload for up to 30 seconds without being re-prompted for your location, but then after 30 seconds you'll get prompted again
I probably should have just installed loom on safari but here we are:
https://youtu.be/VU5nAQ74pyo
docs:
radarlabs/documentation#232