Add onInit hook, drop json3, update build dependencies #178
Conversation
| this._q = []; // queue for proxied functions before script load | ||
| this._sending = false; | ||
| this._updateScheduled = false; | ||
| this._onInit = []; |
There was a problem hiding this comment.
This needs to be in the snippet right? Similar to the queued functions:
https://github.com/amplitude/Amplitude-JavaScript/blob/master/src/amplitude-snippet.js#L10-L14
The reason is that the AmplitudeClient might not be loaded yet, so you can't use it to queue
https://github.com/amplitude/Amplitude-JavaScript/blob/master/src/index.js#L2-L11
| it('should invoke onInit callbacks', function() { | ||
| let onInitCalled = false; | ||
| let onInit2Called = false; | ||
| amplitude.onInit(() => { onInitCalled = true; }); |
There was a problem hiding this comment.
Another test you might want to do (not sure if you can automate it, might have to manually do it), is verify that the functions are being queued even when the SDK hasn't loaded. So like maybe mess with the src of the snippet so that the SDK doesn't load, and verify that the dud queue still has onInit callbacks.
There was a problem hiding this comment.
Actually we don't need to do anything too complicated. Code within the snippet script tag will always load before the SDK loads from the snippet.
Tested. Looks like it works as hoped.
blazzy
force-pushed
the
onInit
branch
2 times, most recently
from
88b1fca to
2104a86
Compare
djih
left a comment
djih
left a comment
There was a problem hiding this comment.
LGTM, only suggestion is to clear the _onInit queue after running all the functions
| } | ||
|
|
||
| for (let i = 0; i < this._onInit.length; i++) { | ||
| this._onInit[i](); |
There was a problem hiding this comment.
is it worth checking if this a function or something? I guess we don't really do it in when they give us callback functions
There was a problem hiding this comment.
Also might want to clear the _onInit queue after you finish running all the functions, in case they call initialize more than once for whatever reason...
There was a problem hiding this comment.
Going to punt on the type-check, but will add logic to clear the queue.
No description provided.