search

segmentio / analytics.js-integrations

Monorepo housing Segment's analytics.js integrations
https://segment.com/docs/destinations/
MIT License
119 stars 141 forks source link

Amplitude and Madkudu integrations break with SystemJS #761

Open robmosca opened 1 year ago

robmosca commented 1 year ago

When trying to initialize Amplitude Classic or Madkudu in a website using SystemJS, the initialization fails with the following message:

analytics.min.js:1 Madkudu TypeError: window.require is not a function

This is due to the fact that the condition on this line is true for SystemJS, but SystemJS does not define window.require (see here and here).

Maybe to the condition on line 13 a check should be added for window.require to be defined.

telmaantunes commented 1 year ago

I've got the same issue for the bugsnag integration. What was your solution?

robmosca commented 1 year ago

Unfortunately, disabling the integrations... :( For amplitude we moved to server side integration. Madkudu we just disabled it.

robmosca commented 1 year ago

The fix should be as easy as changing the following lines: https://github.com/segmentio/analytics.js-integrations/blob/548c10c110f9514b3d8a84ef1f1db8a5ff81e488/integrations/amplitude/lib/index.js#L19 https://github.com/segmentio/analytics.js-integrations/blob/548c10c110f9514b3d8a84ef1f1db8a5ff81e488/integrations/bugsnag/lib/index.js#L15 https://github.com/segmentio/analytics.js-integrations/blob/548c10c110f9514b3d8a84ef1f1db8a5ff81e488/integrations/madkudu/lib/index.js#L13

in this way

var umd = typeof window.define === 'function' && window.define.amd;

to

var umd = typeof window.define === 'function' && window.define.amd && window.require;
GhassenRjab commented 1 year ago

Hey @robmosca,

I think replacing

var umd = typeof window.define === 'function' && window.define.amd;

with 

var umd = typeof window.define === 'function' && window.define.amd && window.require;

won't fix the issue.

var umd will have a falsy value and the test on line 44 won't pass. Hence, MadKudu.js won't be loaded.

I am looking at the SystemJS documentation in order to see how we can import MadKudu.js here.

robmosca commented 1 year ago

@GhassenRjab: the test on line 44 won't pass and MadKudu.js will instead be loaded via the traditional way (script element injection) at line 59.

The test at line 44 is just to use RequireJS instead of load() (hence the return statement on line 56).

Or maybe I misunderstand the code? 🤔

GhassenRjab commented 1 year ago

You're right! Nice catch!

I'll open a PR with the suggested solution

robmosca commented 1 year ago

Yeah, I also tested it in Chrome overriding the code of the integration with the proposed change and it seems to work. 👍🏽 Thanks @GhassenRjab!

GhassenRjab commented 1 year ago

@robmosca the PR has been merged. I think a new version has been released