add ability to capture customer events before library init #436

silesky · 2022-04-27T17:27:18Z

Any user method calls to .track / .identify / etc before analytics will be cached until analytics is finished loading.

One of the benefits of the snippet is that If a user calls analytics.track, we capture events for that user and then flush those events once the library has finished initializing.

However, when using the npm library, a user must currently wait for a .load in order to capture .track calls. This leads to
additional boilerplate as well as scenarios where user events might not get captured, as initialization can take a while. This feature offers a synchronous API that will cache events and then dispatch them when analytics finishes loading.

Other notes:

this includes a fix for snippet users where both an .on event and its callback (e.g on('track', callback) will fire twice if done before init.

This PR allows you to do

import { AnalyticsBrowser } from '@segment/analytics-next'
import { useEffect, useState } from 'react'

// synchronous
const analytics = analyticsBrowser.load({writeKey}) // this loads an instance of AnalyticsBuffer 

const App = () => {
  return (
    <div>
        <button
          onClick={(e) => analytics.track("hello world", {prop: 1})}>
        >
          Track
        </button>
    </div>
  )
}

This change is backwards-compatible with the previous API.
Awaiting on AnalyticsBrowser will still return you [Analytics, Context] (like before).

import { AnalyticsBrowser } from '@segment/analytics-next'
import { useEffect, useState } from 'react'

const App = () => {
  const [analytics, setAnalytics] = useState(undefined)
  useEffect(() => {
    const loadAnalytics = async () => {
      const [analytics] = await AnalyticsBrowser.load({ writeKey })
      setAnalytics(analytics)
    }
    loadAnalytics()
  }, [])

  return (
    <div>
        <button
          onClick={(e) => analytics?.track("hello world", {prop: 1})}>
        >
          Track
        </button>
    </div>
  )
}

src/__tests__/standalone-analytics.test.ts

example/pages/index.tsx

src/__tests__/pre-init.integration.test.ts

arielsilvestri · 2022-04-27T21:33:42Z

src/browser.ts

+/**
+ *  Represents a buffered method call that occurred before initialization.
+ */
+interface PreInitMethodCall<T extends PreInitMethodName = PreInitMethodName> {


I'm unfamiliar with the section that follows the extends keyword here. What's happening there?

Sure; it assigns the default type parameter of PreInitMethodName, so PreInitMethodCall can be called without an argument (if desired).

Created a playground!

https://www.typescriptlang.org/play?#code/C4TwDgpgBAaghgGwK7QLxQOQDMD2ONQA+mARnAE4YBQVokUAYgDwAqUEAHsBAHYAmAZ1iIUUdPGQQAfGKgBvKlCVQAbiIgAuKC0XKeSALZaAjFQC+NAMY4eA4FBwkAVsa0Ml6BctXqtAIjJyPwAaXSV9IyhTCyprW3tHJwAmNyZsPAwZWS9lNUl-QJCwqAiTcxogA

BTW, I just pushed a commit that simplified the types a bit!

Oh I didn't know you could do default values in generics, neat!

chrisradek · 2022-04-27T21:56:10Z

I think this feature has a lot of merit and I like the improvements, especially that track et al. now returns a promise in all cases.

I also like that this allows calling methods on a single object pre and post initialization. I think you could modify this a bit to avoid having to add a bunch of static properties to AnalyticsBrowser and treat it as a singleton.

If we exposed something like a preload() method, that could return a stubbed Analytics that calls the real deal after load() is called on it. So instead of stubbing those methods on AnalyticsBrowser and then forwarding calls to the real Analytics when instance exists, you'd be stubbing a new object returned by preload() instead.

The benefits I see here are:

Limit the surface of new APIs. This would be just the addition of 1-2 new APIs.
Prevents treating AnalyticsBrowser as a singleton.
Allows for multiple instances in the future - we've gone back and forth on this, but if we treat AnalyticsBrowser as a singleton we make it a lot harder to support if we decide to in the future.

I haven't gone through the code in depth, but I wanted to get your thoughts on this before digging deeper.

arielsilvestri · 2022-04-27T22:27:08Z

src/browser.ts

+    // this guard is probably not needed.
+    if (typeof analytics[method] !== 'function') {
+      return console.warn(
+        `invariant error: method call "${method}" does not exist on analytics instance: ${analytics}`
+      )
+    }


Hmm, I'm also struggling to figure out how this could happen.

It was a guard in the existing code! I do think it's an impossible state; if the method doesn't exist, the user will get a reference error.

Yeah, probably almost impossible state. Maybe if someone uses a snippet with invalid methods, or manually add 💩 to the buffer.

Yeah, probably almost impossible state. Maybe if someone uses a snippet with invalid methods, or manually add 💩 to the buffer.

In that case, I think they’ve really earned this error message!

It's possible that prior to your changes, maybe certain functions wouldn't exist for some edge case, but with all your stricter typings and setup, I think dropping this from future implementations is fine

silesky · 2022-04-27T23:13:39Z

I think this feature has a lot of merit and I like the improvements, especially that track et al. now returns a promise in all cases.

I also like that this allows calling methods on a single object pre and post initialization. I think you could modify this a bit to avoid having to add a bunch of static properties to AnalyticsBrowser and treat it as a singleton.

If we exposed something like a preload() method, that could return a stubbed Analytics that calls the real deal after load() is called on it. So instead of stubbing those methods on AnalyticsBrowser and then forwarding calls to the real Analytics when instance exists, you'd be stubbing a new object returned by preload() instead.

The benefits I see here are:

Limit the surface of new APIs. This would be just the addition of 1-2 new APIs.

Prevents treating AnalyticsBrowser as a singleton.

Allows for multiple instances in the future - we've gone back and forth on this, but if we treat AnalyticsBrowser as a singleton we make it a lot harder to support if we decide to in the future.

I haven't gone through the code in depth, but I wanted to get your thoughts on this before digging deeper.

@chrisradek Thanks for looking this over and writing/voicing your thoughts, and taking the time to chat today! I look forward to talking more about the API and where this fits in with multi-instance and AnalyticsNode.

Just to respond to the smaller points (for anyone else reviewing)

This feature is not intended to add any overhead (besides the warning) to implementing multi-instance in the future if we imagine that the static method API is documented as only for singleton users . I do think trying to use the static API for multi instance users is possible, but does not seem to be in the spirit of the feature. I could imagine a multi-instance API that requires analytics.load for the users who need it (maybe returning a tuple, maybe just supporting multi instances), and is similar to the older analytics-next instantiation API.
The warning is a bit controversial, I saw it as not actually a stop sign, just a kind of "this is most likely a common bug" oops, they are calling .load multiple times -- better move that somewhere else! Inspired by "You have included the Google Maps API multiple times on this page. This may cause unexpected errors." I have probably dozens of mistakes like this in my life in front-end applications. Basically acknowledging that the 95% use a single instance, and this warning might be helpful. Other options: add a suppressMultiWarning option, or possible to make it a "development-build only" message, similar to development-only react library warnings. Or, for multi instance users where the warning is a false or detect multi-instance configuration and suppress it that way. Lots of possibilities.

pooyaj

Did the first pass, amazing Typescript 🧙‍♂️ two general comments:

1- If we don't change the implementation, I wonder from usage standpoint we can suggest something like this as well ( in case someone doesn't like to use the class/static methods post-load or to do multi-instance, pretty much gives user the pre-buffer access instantly, and they eventually get access to the instance after load ):

  const [analytics, setAnalytics] = useState(AnalyticsBrowser)
  useEffect(() => {
    const loadAnalytics = async () => {
      let [response] = await AnalyticsBrowser.load({ writeKey })
      setAnalytics(response)
      AnalyticsBrowser_resetGlobalState()
    }
    loadAnalytics()
  }, [writeKey])

(only concern, not sure how happy Typescript will be with the above ☝️)

2- Probably the only concern is bundle-size, not sure how much we can squeeze down 😬

pooyaj · 2022-04-28T02:07:35Z

src/browser.ts

+    // this guard is probably not needed.
+    if (typeof analytics[method] !== 'function') {
+      return console.warn(
+        `invariant error: method call "${method}" does not exist on analytics instance: ${analytics}`
+      )
+    }


Yeah, probably almost impossible state. Maybe if someone uses a snippet with invalid methods, or manually add 💩 to the buffer.

pooyaj · 2022-04-28T02:14:50Z

src/browser.ts

-  if (anon) {
-    const [, id] = anon
-    analytics.setAnonymousId(id)
+async function flushPreBuffer(


I think some opportunity for optimization here (I know it is old code), but given on and setAnonymousId aren't async we can filter for both setAnonymousId and on in one go, and then call all of them sequentially in a forEach

This is a good callout -- based on how it was written, I wasn't sure if setAnonymousId had some magic that required that order of operations, so I preserved the original structure.

I made it a bit more functional instead of using a foreach -- it's still O(n) -- let me know your thoughts.

yordis · 2022-04-28T02:49:38Z

src/browser.ts

+  /**
+   * Analytics class instance initialized by AnalyticsBrowser.load.
+   * Reinitializes everytime load is called.
+   */
+  static instance?: Analytics = undefined
+  /**
+   * Analytics context instance initialized by AnalyticsBrowser.load.
+   */
+  static ctx?: Context = undefined
+
+  static preInitBuffer = new PreInitMethodCallBuffer()
+
+  /**
+   * Clear the global state. Useful mainly for testing.
+   */
+  static _resetGlobalState() {


Related to: #391

I wish the codebase start to move away from these classes, especially if every method is a static method or variable.

Such practices make the codebase less tree-shakeable and treat the class as a bag of things (a file module) when you can achieve the same doing import * as MyName from '...' if you care about the aesthetic aspect of it.
Also, it is quite hard to follow, in the FP version you would have clear boundaries with data, state, and behavior.

I made the PR to showcase what it will look like and hopefully you would move there sooner than later. (Sentry@v7 is doing a lot of work on that front if you want validation, your products are really close when it comes to SDKs)

The bigger this class keeps getting, the harder it will be replace it or justify the breaking change, and the Sunk Cost Fallacy will be a thing.

My :2cents:

Just chiming in with some context in regards to the Sentry side (https://github.com/getsentry/sentry-javascript)!

Static methods and attributes in classes are something we've stayed away from in our Browser SDK primarily because they just can't get minified. Something like AnalyticsBrowser.trackSubmit() minifies to aa.trackSubmit(), vs. for a function -> function trackSubmit() -> function aa(). This adds up over time (and suddenly makes function name length actually matter, never a good convo to have in code reviews). As an unrelated note, notice how this also applies for all objects in general, which makes destructuring and object keys all have a cost as well.

This is further exacerbated by the fact that the bundle increases even more when classes are transpiled down to es5 (though this is more an argument to just not use classes at all).

Then there's the general problem of bringing in unrelated logic whenever you import in the class to use static methods, but that is generally solved by keeping classes small + using dependency injection as much as possible (also just not using classes at all).

In general, we've had a lot of gains in moving private/static methods outside of a class (with shims for backwards compatibility until we can make the breaking change). See getsentry/sentry-javascript#4281 as an example!

If you have any questions, please ping, happy to help out :)

Related to: #391

I wish the codebase start to move away from these classes, especially if every method is a static method or variable.

Such practices make the codebase less tree-shakeable and treat the class as a bag of things (a file module) when you can achieve the same doing import * as MyName from '...' if you care about the aesthetic aspect of it. Also, it is quite hard to follow, in the FP version you would have clear boundaries with data, state, and behavior.

I made the PR to showcase what it will look like and hopefully you would move there sooner than later. (Sentry@v7 is doing a lot of work on that front if you want validation, your products are really close when it comes to SDKs)

The bigger this class keeps getting, the harder it will be replace it or justify the breaking change, and the Sunk Cost Fallacy will be a thing.

My :2cents:

Somewhat related — I did some naive comparison testing in our playground using webpack-bundle-analyzer before / after (master vs this branch) and there was essentially no difference in the bundle size.

As I pointed out here: #391 is just a simple change already reducing the bundle size. Is it minimal? yes, most definitely. But for such minimal work that is exactly the same just changing the habits already pays off. As an application developer, I would appreciate every byte you save.

I could continue with the rabbit hole, as @AbhiPrasad pointed out, Sentry already proved some of these concepts with huge benefits.

I can try to do what I did on getsentry/sentry-javascript#4381 if you need more proof that it will eventually compound.

If I am not mistaken Sentry has seen up to 24% reduction and that is not even counting optimizations that you could continue doing, nor counting that it is not 100% of the work that could be done.

Anyway, it could a discussion for later, but as I pointed out "Sunk Cost" may make things harder to change, we are all humans after all.

❤️

As usual, thanks @yordis for the input. I don't think the static methods precludes us from refactoring to a more "functional" and "tree-shaking" friendly interface (per your previous PR).

I can image us eventually refactoring the library to have an entry point that is something like:

export { track } from './track' export { load } from './load' // ... more methods // for legacy users -- it looks like a class, but it's just a POJO export const AnalyticsBrowser = { load, // or dynamic import? track, // ... more methods }

And thank you @AbhiPrasad, these is great feedback and I will definitely look @sentry's SDK moving forward.

I’m available on Sentry’s discord if you ever need anything.

Also apologizes if I came across like I was bikeshedding or causing friction on your PR - totally not the intention, just trying to share some learnings (and hopefully learn some stuff from you folks as well!)

@AbhiPrasad no; you weren’t bikeshedding at all, this is great stuff to bring up!

chrisradek · 2022-04-28T05:08:32Z

@silesky I've been thinking about this more.

My main concerns about treating AnalyticsBrowser as the analytics class:

It's not obvious that you have to call load() before the events start getting sent. Perhaps documentation is enough and at least it should be obvious that events aren't getting sent during dev.
Calling load() multiple times causes unexpected results. What happens if I call load() a 2nd time with a new writeKey? Does that overwrite the instance that AnalyticsBrowser uses? (Currently it looks like yes). That can cause problems with loading destinations.

I suspect this new strategy will end up being what's used by consumers bundling the library in their application, so I just want to make sure we think about and limit opportunities for devs to accidentally do the wrong thing.

On a separate note I do agree with @yordis about not using classes with only static methods since that makes it harder for tools to do some tree-shaking. For example, analytics.ts and standalone-analytics.ts have a reference to AnalyticsBrowser.load(). With this change, the standalone bundler will end up pulling in all the static methods on AnalyticsBrowser that are added. If load() were its own method and we referenced that directly instead of AnalyticsBrowser.load I bet it'd reduce our bundle size compared to what it is with your changes here.

silesky · 2022-04-28T19:18:44Z

It’s not obvious that you have to call load() before the events start getting sent. Perhaps documentation is enough and at least it should be obvious that events aren’t getting sent during dev.
Calling load() multiple times causes unexpected results. What happens if I call load() a 2nd time with a new writeKey? Does that overwrite the instance that AnalyticsBrowser uses? (Currently it looks like yes). That can cause problems with loading destinations.
I suspect this new strategy will end up being what’s used by consumers bundling the library in their application, so I just want to make sure we think about and limit opportunities for devs to accidentally do the wrong thing.

I think it’s common for web developers using a library to initialize something once at the beginning of their application. They can even call it outside of the scope of a component.

AnalyticsBrowser.load()

const App = () => …

Most database / ORM clients (as default behavior) expect a user to connect once early for the duration of the application. For example, mongoose has a similar API, ditto Apollo Client, etc. This feature has no new behavior in that regard, and will have no affect on any existing users, other than they might be alerted if they're using unsupported behavior.

On a separate note I do agree with @yordis about not using classes with only static methods since that makes it harder for tools to do some tree-shaking. For example, analytics.ts and standalone-analytics.ts have a reference to AnalyticsBrowser.load(). With this change, the standalone bundler will end up pulling in all the static methods on AnalyticsBrowser that are added. If load() were its own method and we referenced that directly instead of AnalyticsBrowser.load I bet it’d reduce our bundle size compared to what it is with your changes here.

I’m not so sure about this actually — AnalyticsBrowser.load still returns an instance (even if not referenced) and that code would still be part of the bundle.

Somewhat related — I did some naive comparison testing in our playground using webpack-bundle-analyzer before / after (master vs this branch) and there was essentially no difference in the bundle size. My hunch is that, while we might be able to save some bytes here or there, unlike, say, lodash or ramda, the number of functions / methods in this library is quite small but that each public API method has a large amounts of shared logic (e.g. priority queue) that can’t neccessarily be tree-shook away — at least, not without other refactoring and API changes that’s out of the scope of this issue (e.g. moving away from classes completely, moving all the analytics methods to external functions).

Minification is a separate issue that I'm sympathetic to -- but for something like method names, it's still a savings on the level of bytes. That can be important, I agree. However, I think we it's something wen iterate on. The bundle size and structure can be improved in the future, but it's much harder to change the public API, so I think it'd be worth it to focus on 'what's the best public API for the majority of our users' that and then let the other "optimization" discussion follow from that.

arielsilvestri · 2022-04-28T19:20:41Z

Would the analyzer be able to account for tree-shaking when externally consumed? or is it all just using locally built bundles when testing things out?

silesky · 2022-04-28T19:33:55Z

Would the analyzer be able to account for tree-shaking when externally consumed? or is it all just using locally built bundles when testing things out?

I don't 100% know. Unless we did something in our webpack prod build that was getting in the way, the analyzer would be able to tree shake local dependencies just as if they were external. The size will be off, but it works for comparison. If we wanted to be sophisticated, we'd have to test using rollup and webpack under a range of different conditions.

However, I just did this for fun -- there's not a reason why they would be different. If you're using classes in general, you're not thinking about tree shaking. My intuition says that adding static references to "track" etc are not going to increase bundle size beyond the method signature, since "track" etc are always being referenced in the same context in the instance returned by .load.

yordis · 2022-04-28T19:34:58Z

Somewhat related — I did some naive comparison testing in our playground using webpack-bundle-analyzer before / after (master vs this branch) and there was essentially no difference in the bundle size.

As I pointed out here: #391 is just a simple change already reducing the bundle size. Is it minimal? yes, most definitely. But for such minimal work that is exactly the same just changing the habits already pays off. As an application developer, I would appreciate every byte you save.

I could continue with the rabbit hole, as @AbhiPrasad pointed out, Sentry already proved some of these concepts with huge benefits.

I can try to do what I did on getsentry/sentry-javascript#4381 if you need more proof that it will eventually compound.

If I am not mistaken Sentry has seen up to 24% reduction and that is not even counting optimizations that you could continue doing, nor counting that it is not 100% of the work that could be done.

Anyway, it could a discussion for later, but as I pointed out "Sunk Cost" may make things harder to change, we are all humans after all.

❤️

chrisradek · 2022-04-28T19:55:23Z

I’m not so sure about this actually — AnalyticsBrowser.load still returns an instance (even if not referenced) and that code would still be part of the bundle.

For what it's worth, I was thinking more about the bundle we serve via our CDN. Taking what's on master and changing the static methods load and standalone to exported functions reduced the final bundle size ~20 bytes, not huge but pretty much free.

It looks like the bundle size for the CDN build with this PR increased ~550 bytes - about 2.2% increase which isn't huge but also for a feature not used by the CDN build. If there are improvements to buffering for the snippet included there then I'd concede the size increase for the CDN build is worth it.

silesky · 2022-04-28T20:19:35Z

I’m not so sure about this actually — AnalyticsBrowser.load still returns an instance (even if not referenced) and that code would still be part of the bundle.

For what it's worth, I was thinking more about the bundle we serve via our CDN. Taking what's on master and changing the static methods load and standalone to exported functions reduced the final bundle size ~20 bytes, not huge but pretty much free.

It looks like the bundle size for the CDN build with this PR increased ~550 bytes - about 2.2% increase which isn't huge but also for a feature not used by the CDN build. If there are improvements to buffering for the snippet included there then I'd concede the size increase for the CDN build is worth it.

Actually we do have a bug fix for the CDN that is related to the new code. =) You can repro this super easily on any analytics site: includes fix for snippet users where both an .on event and its callback (e.g on('track', callback) will fire twice if done before init.

Part of that bugfix (and general clean up) is the additional normalization for the snippet so we can use a single interface that represents a method call type (the PreInitBuffer class etc). This is just the extra mapping code you always need if you want two separate interfaces to share some logic.

Buy yep, I agree we could shave some bytes off if we didn't use static methods. But ultimately , there are much bigger optimizations we can take if we're byte shaving, and I think this should just come down to which API we prefer =)

silesky · 2022-04-29T05:44:21Z

As I pointed out here: #391 is just a simple change already reducing the bundle size. Is it minimal? yes, most definitely. But for such minimal work that is exactly the same just changing the habits already pays off. As an application developer, I would appreciate every byte you save.

I could continue with the rabbit hole, as @AbhiPrasad pointed out, Sentry already proved some of these concepts with huge benefits.

I can try to do what I did on getsentry/sentry-javascript#4381 if you need more proof that it will eventually compound.

If I am not mistaken Sentry has seen up to 24% reduction and that is not even counting optimizations that you could continue doing, nor counting that it is not 100% of the work that could be done.

Anyway, it could a discussion for later, but as I pointed out "Sunk Cost" may make things harder to change, we are all humans after all.

❤️

I appreciate the optimization and bringing up size budget, and I think that there is work to be done there in the near future. I am new to this library and am not its architect, so I am learning as I go. These conversations are super important for the health of this repo, and I appreciate them. I would like to continuously revisit the structure of this repo and improve it -- not only for reasons of performance and the public API, but readability, coherence, encapsulation, etc.

To get us back to this PR: I just did a comparison: Out of the 500 ish bytes that this feature represents, the static methods themselves added 16 bytes to the size of the umd bundle. So really, the references themselves don't take up much space -- though it was worth investigating!

The question I want to focus on is "do people like the API? Does it feel natural, how does it fit in with the future roadmap, is it mantainable?". My personal feeling that it does feel natural and it's quite consistent with the API of other libraries and brings us closer to the "load and forget" standalone (CDN) analytics library, and from what I have articulated, I don't think it limits future opportunities for refactoring or limit our roadmap (such as preventing us from making the API more functional, as you proposed in your PR).

Stepping into the shoes of the "98%" dev (I've addressed the other 2% in earlier posts), I think it represents a large simplification / boilerplate reduction for my entire app, as well as finally allowing me to capture events that were missed because they occurred before load time. That seems like a big win!

yordis · 2022-04-29T19:10:09Z

Leaving some notes,

static isn't that concerning (at the moment), but the programming style could continue optimizing things with simple changes.
Instead of thinking about "Classes" first, it would be much better to lean on typespec, and simple JS Objects, and do it more functional.

For example, PreInitMethodCallBuffer could become an object, and every permutation and mutations and whatnot could be a function that takes a PreInitMethodCallBuffer argument.

Making things classes will continue increasing the bundle size. This could be avoided, especially in cases that they are internal to your lib, so you have full control over what is going on inside.

Anyway, probably we could (if you want to), continue the conversation somewhere else, focus on getting the features to work first, and then follow up.

Make it pretty 💜

chrisradek

Looking good! Nothing serious, just a few suggestions on test cases and some size limit savings.

chrisradek · 2022-05-16T19:59:00Z

src/browser.ts

+  /**
+   * Clear the global state. Useful mainly for testing.
+   */
+  static _resetGlobalState() {


It looks like_resetGlobalState is only used by tests so we should be able to remove it!

chrisradek · 2022-05-16T21:00:41Z

src/analytics-pre-init.ts

+  addDestinationMiddleware = this._createMethod('addDestinationMiddleware')
+
+  private _createMethod<T extends PreInitMethodName>(methodName: T) {
+    return async (


This method doesn't actually use await, so we can remove the async keyword and save some bytes (~36 from the final gzipped bundle!)

Did not know that there was a cost savings!

If so, we should enable https://eslint.org/docs/rules/require-await

Maybe later, I guess =S

chrisradek · 2022-05-16T22:15:23Z

src/analytics-pre-init.ts

+      analytics[call.method] as Function
+    )(...call.args)
+
+    if (result instanceof Promise) {


I wonder if it would be safer to check if typeof result.then === 'function. I recall running into issues with instanceof checks on Promises when users shimmed their own Promise library (like bluebird), and we're running in environments that will do that shimming. Even if result isn't actually a promise, await would still resolve, we'd just defer in that scenario.

chrisradek · 2022-05-16T22:48:37Z

src/analytics-pre-init.ts

+  push(...calls: PreInitMethodCall[]): void {
+    calls.forEach((el) => {
+      if (this._value[el.method]) {
+        this._value[el.method]?.push(el)


It makes me sad that TypeScript can't tell that this value has to be an array at this point. Might be the rare time a non-null assertion makes sense :-/

There is a refactoring but I think I rolled it back because IIRC it was involved -- It comes down to the MethodMap type being a bit loose. =)

chrisradek · 2022-05-16T22:51:19Z

src/analytics-pre-init.ts

+  constructor(calls?: PreInitMethodCall[]) {
+    if (calls) {
+      this.push(...calls)
+    }
+  }


I just saw this constructor get added but nothing uses it. Can it be removed?

I used it for a test. It's nice to be able to initialize inside the constructor, but I can remove.

chrisradek · 2022-05-16T22:58:26Z

src/__tests__/analytics-pre-init.integration.test.ts

+        const newPromise = ajsBuffered.catch((reason) => {
+          expect(reason).toBe(errMsg)
+        })


Does the same work if we're using await e.g. await AnalyticsBrowser.load({ writeKey: 'invalid' }) - does this throw an error?

Yep. For example, that case is also covered here: https://github.com/segmentio/analytics-next/pull/436/files/f1042bc17a4f13b7335433390d1892e77b630aed#diff-a3756d7d2cc9639c2f00f5871656d78a5aa10fddd6ba6bdae04447a577140d4aR165 (and probably some other places)

I think I only preferred to test promise syntax here in order to be explicit that we are talking about thenable methods, since try/catch can be used in other contexts. Wasn't necessary to do it one way or another... Technically, if an object is asserted to be a thenable as it is here (it has a .then and optional .catch method), I wouldn't necessarily try to test both syntax forms, as that's part of the spec.

src/__tests__/analytics-pre-init.integration.test.ts

chrisradek

Whoo! Looks good to me!

pooyaj

🚀🚀🚀

silesky requested review from chrisradek, pooyaj, arielsilvestri and zikaari April 27, 2022 17:29

silesky force-pushed the feature/#428-queue-calls-while-awaiting-load branch from 268a3a7 to a7b96e5 Compare April 27, 2022 17:33

silesky commented Apr 27, 2022

View reviewed changes

src/__tests__/standalone-analytics.test.ts Show resolved Hide resolved

silesky force-pushed the feature/#428-queue-calls-while-awaiting-load branch from a7b96e5 to 9d7680b Compare April 27, 2022 17:45

arielsilvestri reviewed Apr 27, 2022

View reviewed changes

example/pages/index.tsx Outdated Show resolved Hide resolved

arielsilvestri reviewed Apr 27, 2022

View reviewed changes

src/__tests__/pre-init.integration.test.ts Outdated Show resolved Hide resolved

arielsilvestri reviewed Apr 27, 2022

View reviewed changes

pooyaj reviewed Apr 28, 2022

View reviewed changes

yordis reviewed Apr 28, 2022

View reviewed changes

silesky force-pushed the feature/#428-queue-calls-while-awaiting-load branch 6 times, most recently from 8d78c6c to fe4e3e0 Compare April 30, 2022 20:31

silesky requested a review from chrisradek May 16, 2022 21:04

silesky force-pushed the feature/#428-queue-calls-while-awaiting-load branch 2 times, most recently from 4072745 to 956a5f8 Compare May 16, 2022 22:27

refactor typedef tests

9c02f90

silesky force-pushed the feature/#428-queue-calls-while-awaiting-load branch from 956a5f8 to 9c02f90 Compare May 16, 2022 22:36

test naming clean up

f1042bc

chrisradek reviewed May 16, 2022

View reviewed changes

silesky and others added 4 commits May 17, 2022 11:40

remove resetGlobalState

7a064c4

clarify test

cd278d6

add "this" test to event emitter

e77db68

update pre-init buffer, add tests

f785c64

silesky force-pushed the feature/#428-queue-calls-while-awaiting-load branch from 1e1db10 to f785c64 Compare May 17, 2022 18:44

silesky added 2 commits May 17, 2022 17:42

check thenable instead of instanceof promise

6e94ff5

remove async

39f0f27

chrisradek approved these changes May 18, 2022

View reviewed changes

silesky and others added 2 commits May 18, 2022 14:21

update thenable

4d2bddf

add more "this" tests

95ef70c

silesky force-pushed the feature/#428-queue-calls-while-awaiting-load branch from 70deee9 to 95ef70c Compare May 18, 2022 20:56

silesky added 4 commits May 18, 2022 15:59

delete clearAllMocks since clearMocks option is true in jest config

4c2dccb

make push a bit clearer

cdea3d7

get rid of .call since we have a test around it

4401356

rename and move files into core

37cd27a

pooyaj approved these changes May 18, 2022

View reviewed changes

silesky force-pushed the feature/#428-queue-calls-while-awaiting-load branch 3 times, most recently from 2930c9b to 37cd27a Compare May 18, 2022 22:31

encapsulate snippet logic

9a534db

silesky merged commit f524739 into master May 18, 2022

silesky deleted the feature/#428-queue-calls-while-awaiting-load branch May 18, 2022 22:50

silesky mentioned this pull request May 19, 2022

Queue calls while awaiting load when using the NPM package #428

Closed

add ability to capture customer events before library init #436

add ability to capture customer events before library init #436

Conversation

silesky commented Apr 27, 2022 • edited

Choose a reason for hiding this comment

silesky Apr 27, 2022 • edited

Choose a reason for hiding this comment

silesky Apr 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisradek commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Apr 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky commented Apr 27, 2022 • edited

pooyaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Apr 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisradek commented Apr 28, 2022

silesky commented Apr 28, 2022 • edited

arielsilvestri commented Apr 28, 2022

silesky commented Apr 28, 2022

yordis commented Apr 28, 2022

chrisradek commented Apr 28, 2022

silesky commented Apr 28, 2022 • edited

silesky commented Apr 29, 2022 • edited

yordis commented Apr 29, 2022

chrisradek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky May 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky May 17, 2022 • edited

Choose a reason for hiding this comment

chrisradek left a comment

Choose a reason for hiding this comment

pooyaj left a comment

Choose a reason for hiding this comment

silesky commented Apr 27, 2022 •

edited

silesky Apr 27, 2022 •

edited

silesky Apr 27, 2022 •

edited

silesky Apr 28, 2022 •

edited

silesky commented Apr 27, 2022 •

edited

silesky Apr 30, 2022 •

edited

silesky commented Apr 28, 2022 •

edited

silesky commented Apr 28, 2022 •

edited

silesky commented Apr 29, 2022 •

edited

silesky May 17, 2022 •

edited

silesky May 17, 2022 •

edited