segmentio / analytics-next

Segment Analytics.js 2.0
https://segment.com/docs/connections/sources/catalog/libraries/website/javascript
MIT License
405 stars 136 forks source link

Load Breaks with Privacy Badger #554

Open KeKs0r opened 2 years ago

KeKs0r commented 2 years ago

It seems that Privacy Badger blocks the call to get the settings. The problem is, that means that the load call throws. And i have in my code the analytics as a var in the global space

  export const analytics = SEGMENT_TOKEN
            ? await AnalyticsBrowser.load({ writeKey: SEGMENT_TOKEN })
            : undefined

I am currently failing to wrap this call in a try catch. Because the error is async. So in order to "catch" it, I would need to make analytics a Promise instead of the AnalyticsBrowser. Which then means, I have to make all calls to it async.

I think it would be beneficial to handle those kind of errors within the sdk, to prevent breaking user code. This is the error that breaks my app: https://github.com/segmentio/analytics-next/blob/7ea4c607228740fc0ff6ba6d89d9f94705e7daa7/packages/browser/src/browser/index.ts#L85-L91

chrisradek commented 2 years ago

@KeKs0r Thanks for creating this issue!

Can you update the code to include a catch handler like this?

  export const analytics = SEGMENT_TOKEN
            ? await AnalyticsBrowser.load({ writeKey: SEGMENT_TOKEN }).catch(() => undefined)
            : undefined

That will prevent errors from breaking your code - instead undefined is returned if an error is encountered. If there's no error, then analytics gets returned the awaited value of load.

AnalyticsBrowser.load() actually returns a tuple when you await it so you probably want something like:

  export const analytics = SEGMENT_TOKEN
            ? await AnalyticsBrowser.load({ writeKey: SEGMENT_TOKEN }).then(([analytics]) => analytics).catch(() => undefined)
            : undefined
silesky commented 2 years ago

Seems like a potentially common issue with blockers; we might want to just noop this kind of fetch error by default. I imagine most Segment users would prefer that this library avoid throwing wherever possible, especially when it’s due to something like this.

KeKs0r commented 2 years ago

@chrisradek this is actually what I tried to do. But adding a "then" or "catch" on the load call, will turn it into a promise. And I can't call analytics.track() anymore. I would need to change my whole codebase to a (await analyics).track(). And currently all these calls are also in syncronous currently.

@silesky I would agree that this would be the preferred behaviour for me. If a user does not want to be tracked: don't break the application or make me handle it, but rather don't do anything, since its what the user wants anyways.

silesky commented 2 years ago

@KeKs0r ... did you try using the synchronous API?

 export const analytics = SEGMENT_TOKEN
            ? AnalyticsBrowser.load({ writeKey: SEGMENT_TOKEN })
            : undefined

analytics.track('foo')

It should log load errors, but should not throw.

edit: I added a test to confirm this behavior https://github.com/segmentio/analytics-next/pull/557/files#diff-47514c4c862cd4b0072c15f4bb644190a93539c5c089f647fd418239b4f928a4R189

KeKs0r commented 2 years ago

@silesky yes I ran into this in production. Also updated to the newest version of this library. Only way to get around it in my codebase was to remove the throw part in the catch.

Maybe it also depends on the bundling, since my analytics is defined on the top level of a module. So depending on how a bundler might wrap the file import, it potentially throws on a top level.

prsnca commented 1 year ago

@silesky My client breaks with an adblocker is present (uBlock origin in my case) - it blocks our Hubspot integration (The URL is from this format https://js.hs-analytics.net/analytics/XXXXX/XXX.js The weird thing is that we do not even get into the catch clause on failure, the application hangs and does not continue to load. We are doing it synchronously using await since we want to start sending events right away.

Any suggestions here?

Thanks!

silesky commented 1 year ago

If we're still just talking about catching any initial load errors and doing something with them (besides console logging), the new, documented way is:

export const analytics = new AnalyticsBrowser();
analytics
  .load({ writeKey: "MY_WRITE_KEY" })
  .catch((err) => ...);

Note: this catch clause will not handle device mode script blocked errors -- the only adblocking errors that it should handle would be if the settings CDN gets blocked.

I am interested in this adblocker case -- some questions:

@prsnca

  1. it's only blocking the hubspot script, but not anything else (typically segment's cdn is the thing that gets blocked) ?

  2. When the hubspot script gets blocked, does it take down your entire application / page; or, is it just that the analytics client does not send any events to Segment, but things work as usual?

  3. Can you post your analytics initialization code?

prsnca commented 1 year ago

We have tried to upgrade the client to the latest version but still could not catch the failure under that catch clause which is very weird... this is how we do it today - causes the application to hang and the failure does not get to the catch part... also tried to debug it but couldn't get to the bottom of the error.

try {
            const [analytics] = await AnalyticsBrowser.load(
                {
                    writeKey: segmentWriteKey,
                    cdnURL: SEGMENT_PROXY_CDN,
                },
                {
                    initialPageview: false,
                    integrations: {
                        'Segment.io': { apiHost: SEGMENT_PROXY_API },
                    }
                }
            );
            this.segmentClient = analytics;
        } catch (err: any) {
            console.log('Error initializing Segment', err);
            return;
        }

upgraded to this, and still can't get to the catch clause on failure:

try {
    AnalyticsBrowser.load({
                    writeKey: segmentWriteKey,
                    cdnURL: SEGMENT_PROXY_CDN,
                },
                {
                    initialPageview: false,
                    integrations: {
                        'Segment.io': { apiHost: SEGMENT_PROXY_API },
                    }
                })
                .then(async ([analytics, _context]: [Analytics, Context]) => {
                    this.segmentClient = analytics;
                })
                .catch(() => undefined);
        } catch (err: any) {
            console.log('Error initializing Segment', err);
            return;
        }
  1. it's blocking a lot of scripts, but blocking this specific one causes the failure (I played with the adblocker's allowlist)
  2. it just hangs... we're on await there and it never returns - so the application does not continue to start up
  3. ^^

Thanks @silesky!

davidgonor1408 commented 1 year ago

Hey @silesky, having the same issue as @prsnca. Do you have any suggestions? Thanks

erickarnis-tb commented 10 months ago

Try this

const hasKey = Boolean(import.meta.env.SEGMENT_TOKEN)

export const track = async (...args: Parameters<typeof analytics.track>) => {
  if (hasKey) {
    await analytics.track(...args)
  }
}
Skwai commented 6 months ago

I've just encountered this.

The problem I had was that Segment would load but fail to fetch settings (request was blocked by browser).

This would mean that analytics.ready would never resolve.

To solve I had to wrap analytics.user() calls in a timeout closure to handle cases where Segment will have loaded but ready will never have resolved.

export const withTimeout = <T>(promise: () => Promise<T>, timeout: number): Promise<T | undefined> => {
  return new Promise<T>((resolve, reject) => {
    const id = setTimeout(() => {
      reject(new Error('Timed out'))
    }, timeout)

    promise()
      .then((result) => {
        clearTimeout(id)
        resolve(result)
      })
      .catch((error) => {
        clearTimeout(id)
        reject(error)
      })
  })
}

const user = await withTimeout(() => analytics.user(), 2_000)

if (user) {
  console.log(user.anonymousId())
}
georgebutter commented 6 months ago

I have Segment running via a proxy and it's working great based on the instructions here. https://segment.com/docs/connections/sources/catalog/libraries/website/javascript/custom-proxy/

However, the destinations I have set up (for example Mixpanel) also load a cdn (http://cdn.mxpnl.com/libs/mixpanel-2-latest.min.js), which is not proxied and is therefore blocked. Is there a way to use the integrations options to also proxy these requests?

silesky commented 6 months ago

However, the destinations I have set up (for example Mixpanel) also load a cdn (http://cdn.mxpnl.com/libs/mixpanel-2-latest.min.js), which is not proxied and is therefore blocked. Is there a way to use the integrations options to also proxy these requests?

@georgebutter -- analytics.js is just a loader for destinations.

Unless there's some mixpanel global you can modify to update that config, that option would be something that would need to be supported on the action side and likely passed to mixpanel via instantiation. Currently, there's no option for configuring api host / proxy options in mixpanel via segment.

You could make a support request or a contribution to https://github.com/segmentio/action-destinations