search

EmmanuelRoux / ngx-matomo-client

Matomo analytics client for Angular applications
https://matomo.org/
MIT License
74 stars 16 forks source link

trackerUrl required even if scriptUrl should be sufficient #32

Closed jonhamm closed 2 years ago

jonhamm commented 2 years ago

I moved to. this package from another matomo tracker package (https://www.npmjs.com/package/ngx-matomo) Previously I was only required to specify scriptUrl but after changing to this package I am required to specify both scriptUrl and trackerUrl - like this:

{
      provide: MATOMO_CONFIGURATION,
      useValue: {
        disabled: !cue.config.analyticsConfig?.enabled,
        scriptUrl: !!cue.config.analyticsConfig?.enabled
          ? 'https://product-analytics.xxxx.com/analytics/js/container_X4aTUp5k.js'
          : undefined,
        trackerUrl: !!cue.config.analyticsConfig?.enabled
          ? 'https://product-analytics.xxxx.com/analytics/'
          : undefined,
      } as MatomoConfiguration,
 },

If I only specify scriptUrl like this in my app.module 

{
      provide: MATOMO_CONFIGURATION,
      useValue: {
        disabled: !cue.config.analyticsConfig?.enabled,
        scriptUrl:
          'https://product-analytics.xxxx.com/analytics/js/container_X4aTUp5k.js',
      } as MatomoConfiguration,
},

I get this error:

zone.js:372 TypeError: Cannot read properties of undefined (reading 'endsWith')
    at appendTrailingSlash (ngx-matomo-tracker.mjs:1332:16)
    at buildTrackerUrl (ngx-matomo-tracker.mjs:1336:16)
    at MatomoInitializerService.injectMatomoScript (ngx-matomo-tracker.mjs:1368:36)
    at MatomoInitializerService.init (ngx-matomo-tracker.mjs:1362:14)
    at new NgxMatomoTrackerModule (ngx-matomo-tracker.mjs:1432:30)
    at Object.NgxMatomoTrackerModule_Factory [as factory] (ngx-matomo-tracker.mjs:1446:1)
    at R3Injector.hydrate (core.mjs:11457:1)
    at R3Injector.get (core.mjs:11276:1)
    at core.mjs:11314:1
    at Set.forEach (<anonymous>)
    at R3Injector._resolveInjectorDefTypes (core.mjs:11314:1)
    at new NgModuleRef (core.mjs:21821:1)
    at NgModuleFactory.create (core.mjs:21875:1)
    at core.mjs:26016:1
    at ZoneDelegate.invoke (zone.js:372:1)
invoke @ zone.js:372
run @ zone.js:134
(anonymous) @ zone.js:1276
invokeTask @ zone.js:406
runTask @ zone.js:178
drainMicroTaskQueue @ zone.js:582
Promise.then (async)
scheduleMicroTask @ zone.js:565
scheduleTask @ zone.js:396
scheduleTask @ zone.js:221
scheduleMicroTask @ zone.js:241
scheduleResolveOrReject @ zone.js:1266
then @ zone.js:1410
bootstrapModule @ core.mjs:26067
45725 @ main.ts:11
__webpack_require__ @ bootstrap:19
__webpack_exec__ @ postcss.mjs:30
(anonymous) @ postcss.mjs:30
webpackJsonpCallback @ jsonp chunk loading:71
(anonymous) @ main.js:1
EmmanuelRoux commented 2 years ago

Hi @jonhamm

trackerUrl is required but scriptUrl is not, scriptUrl will default to trackerUrl + 'matomo.js' (because that's the common default value for Matomo).

There is no way for this library to guess the suffix /js/container_E4uvUp3I.js. I think it's the same with the other lib you were using before.

Also, the configuration you show here is invalid: trackerUrl and siteId configuration properties are both required. They should have been automatically added for you with a forRoot() call if you installed using ng add as described in documentation. This way, your configuration is strongly typed and missing properties should result in a TS compiler error. Here you get the error Cannot read properties of undefined (reading 'endsWith') because required property trackerUrl is missing, this is normal.

Your configuration should probably look like this:

@NgModule({
  imports: [
    NgxMatomoTrackerModule.forRoot({
      disabled: !cue.config.analyticsConfig?.enabled,
      siteId: '', /* THIS IS REQUIRED */
      trackerUrl: 'https://product-analytics.xxxx.com/analytics/',
      scriptUrl: 'https://product-analytics.xxxx.com/analytics/js/container_E4uvUp3I.js',
    }),
  ],
})
export class AppModule {}

But maybe you have a pre-defined tracker in the JS script? I am not aware of such scenario, but in this case the trackerUrl and siteId would need to be optional.

jonhamm commented 2 years ago

Thanks for the immediate response!

It does work without siteId!

We have made matomo create a script (container_E4uvUp3I.js) with embedded configuration - e.g. trackerUrl and siteId as shown below.

So would it be possible to have the configuration with only scriptUrl in the expectation that the returned script would contain all remaining configuration ??

matomo

khaled4vokalz commented 2 years ago

@EmmanuelRoux , Why would the siteID and trackerURL be required if the matomo container already provides those in the JS file that is served. Any specific reason this package has made it mandatory? at least the other package doesn't force us doing so https://github.com/Arnaud73/ngx-matomo/blob/master/projects/ngx-matomo/src/lib/matomo-injector.service.ts#L58

EmmanuelRoux commented 2 years ago

@jonhamm @khaled4vokalz

Thanks for the feedback. You're right, siteId and trackerUrl should not be required in this case (I wasn't aware of such scenario before).

I should fix it soon when I have some time.

(Feel free to provide a PR! Thanks 😃)

@jonhamm Could you please provide the tracking code generated by Matomo in your example?

jonhamm commented 2 years ago

I have uploaded the file - subsequently removed as it was not useful It is modified to hide URLs and some tags and triggers have been removed as I suspect the ones left will show how it works

EmmanuelRoux commented 2 years ago

Thanks @jonhamm, but I was just asking for the code to integrate (section "Install code" of Tag Manager), not the whole script!

A new version will be published soon

jonhamm commented 2 years ago

This is the installation code 

<!-- Matomo Tag Manager -->
<script>
var _mtm = window._mtm = window._mtm || [];
_mtm.push({'mtm.startTime': (new Date().getTime()), 'event': 'mtm.Start'});
var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0];
g.async=true; g.src='https://product-analytics.xxxxx.com/analytics/js/container_E4uvUp3I.js'; s.parentNode.insertBefore(g,s);
</script>
<!-- End Matomo Tag Manager -->
EmmanuelRoux commented 2 years ago

:tada: This issue has been resolved in version 2.4.0-next.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

EmmanuelRoux commented 2 years ago

@jonhamm Can you please test with pre-release version 2.4.0-next.3 and tell me if it is ok for you?

You can update your dependencies with ng update @ngx-matomo/tracker@2.4.0-next.3

jonhamm commented 2 years ago

I will test ASAP...

jonhamm commented 2 years ago

First observation (need some change to MatomoConfiguration:

Argument of type '{ scriptUrl: string | undefined; }' is not assignable to parameter of type 'MatomoConfiguration'.
  Type '{ scriptUrl: string | undefined; }' is not assignable to type 'BaseMatomoConfiguration & Without<ManualMatomoConfiguration, (Without<ExplicitAutoConfiguration, EmbeddedAutoConfiguration> & BaseAutoMatomoConfiguration & Partial<...>) | (Without<...> & ExplicitAutoConfiguration)> & Without<...> & Partial<...> & Without<...> & MatomoTrackerConfiguration'.
    Type '{ scriptUrl: string | undefined; }' is missing the following properties from type 'MatomoTrackerConfiguration': siteId, trackerUrlts(2345)

when

    NgxMatomoTrackerModule.forRoot({
      scriptUrl: !!cue.config.analyticsConfig?.enabled
        ? 'https://product-analytics.xxxx.com/analytics/js/container_E4uvUp3I.js'
        : undefined,
    }),
EmmanuelRoux commented 2 years ago

That's because this lib has strict types (either trackerUrl+siteId or scriptUrl must be set). Try this config:

NgxMatomoTrackerModule.forRoot({
  disabled: !cue.config.analyticsConfig?.enabled,
  scriptUrl: 'https://xxxxxxxxxxxx.com/analytics/js/container_E4uvUp3I.js',
}),
jonhamm commented 2 years ago

Seems to be working perfect 👏 Thanks a lot 🙇‍♂️

EmmanuelRoux commented 2 years ago

:tada: This issue has been resolved in version 2.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

jonhamm commented 2 years ago

That's because this lib has strict types (either trackerUrl+siteId or scriptUrl must be set). Try this config:

NgxMatomoTrackerModule.forRoot({
  disabled: !cue.config.analyticsConfig?.enabled,
  scriptUrl: 'https://product-analytics.xxxx.com/analytics/js/container_E4uvUp3I.js',
}),

@EmmanuelRoux : could I ask you to xxxx our company name in your reply above

EmmanuelRoux commented 2 years ago

@jonhamm done, I’ve also purged revisions history in all messages

av3000 commented 2 years ago

Hi @EmmanuelRoux wanted to ask, how soon could the Angular 12 version package (1.3.5) be updated with this fix?

EmmanuelRoux commented 2 years ago

Hi @av3000 , it's not currently planned.

But I think it can easily be backported to 1.3.x. Could you provide a PR? You can cherry-pick ee9364d5667121763765b13980b239ca8b610568 with some adaptation (you can omit all schematics-related stuff (ng-add...), as it was not supported in 1.x versions)