Add startTrackingVerified(), getVerifiedLocationToken(), and onTokenUpdated() #178
Conversation
| @@ -78,14 +83,86 @@ class VerifyAPI { | |||
| expiresAt, | |||
| expiresIn, | |||
| passed, | |||
| failureReasons, | |||
| _id, | |||
| } as RadarTrackVerifiedResponse; | ||
|
|
||
| if (options.debug) { | ||
| trackRes.response = response; | ||
| } | ||
|
|
||
| lastToken = trackRes; |
There was a problem hiding this comment.
token is the entire response?
There was a problem hiding this comment.
Yes. Deserialized to RadarVerifiedLocationToken in mobile SDKs
| import type { RadarTrackParams, RadarTrackVerifiedResponse } from '../types'; | ||
| import type { RadarStartTrackingVerifiedParams, RadarTrackParams, RadarTrackVerifiedResponse } from '../types'; | ||
|
|
||
| let tokenTimeoutId: any | null = null; |
There was a problem hiding this comment.
Should we namespace these as verified instead of token?
There was a problem hiding this comment.
Also, it's probably preferable to mark things as optional instead of null
let tokenTimeoutId?: any;
let tokenCallback?: (token: RadarTrackVerifiedResponse) => void;Otherwise it needs to be explicitly set to null
| } as RadarTrackVerifiedResponse; | ||
|
|
||
| if (options.debug) { | ||
| trackRes.response = response; | ||
| } | ||
|
|
||
| lastToken = trackRes; | ||
| lastTokenNow = performance.now(); |
There was a problem hiding this comment.
We should use Date.now() unless there's a need for nano-second measurement (this is really only intended for perf measurement)
There was a problem hiding this comment.
The problem with Date.now() is that you can manipulate it by changing your system clock time, whereas performance.now() is based off of time since page load and harder to tamper with
There was a problem hiding this comment.
We do something similar on the mobile SDKs (SystemClock.elapsedRealTimeNanos on Android) for the same reason
| } | ||
|
|
||
| static setExpectedJurisdiction(countryCode?: string, stateCode?: string) { | ||
| expectedCountryCode = countryCode || null; |
There was a problem hiding this comment.
If these are marked as optional in the definition, we wont need the || null here
There was a problem hiding this comment.
I was getting errors because expectedCountryCode is defined as | null instead of | undefined. https://app.circleci.com/pipelines/github/radarlabs/radar-sdk-js/1241/workflows/a23a44d8-bbed-4f3d-a059-163a7b4407c0/jobs/1016
There was a problem hiding this comment.
I was getting errors because expectedCountryCode is defined as | null instead of | undefined. https://app.circleci.com/pipelines/github/radarlabs/radar-sdk-js/1241/workflows/a23a44d8-bbed-4f3d-a059-163a7b4407c0/jobs/1016
There was a problem hiding this comment.
Yeah, I think you can just define them as:
let expectedCountryCode?: string;
| let tokenCallback: ((token: RadarTrackVerifiedResponse) => void) | null = null; | ||
| let lastToken: RadarTrackVerifiedResponse | null = null; | ||
| let lastTokenNow: number = 0; | ||
| let expectedCountryCode: string | null = null; | ||
| let expectedStateCode: string | null = null; |
There was a problem hiding this comment.
This is probably fine to define the scoped variables like this, but there are a couple issues:
- These wont persist across a page reload
- Can run into issues if there are multiple callers (though I think for track we expect a singleton pattern)
If we need to track the last call time across the page-load, probably need to put it in localstorage.
There was a problem hiding this comment.
Don't need this to work across page load like userId, so should be good. I could move it inside the class if that's important
There was a problem hiding this comment.
If these aren't sensitive it's probably fine. Maybe the user-provided ones should be static variables (callback and expected codes)
| // re-request early to maximize the likelihood that a cached token is available | ||
| if (minInterval > 20) { | ||
| minInterval = minInterval - 10; | ||
| } | ||
|
|
||
| // min interval is 10 seconds | ||
| if (minInterval < 10) { | ||
| minInterval = 10; |
There was a problem hiding this comment.
so if I give the interval param as 21 then it is effectively 11 but if I give 15 it is effectively 15?
There was a problem hiding this comment.
Yeah. I think that's an edge case, but if it bothers us, I can update
|
@david-goodfellow @kochis lmk if any must-fix feedback before 👍 Would love to ship this tonight |
No description provided.