Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

README: fix typos in documentation for setTransport #30

Open
wants to merge 1 commit into
base: v3.x
Choose a base branch
from

Conversation

dbertram
Copy link

@dbertram dbertram commented Nov 9, 2023

We're using the Amplitude GTM template and are trying to "Set the transport to use beacon only when exiting the page".

The code sample from the Browser 2.0 SDK docs is as follows:

window.addEventListener('pagehide',
  () => {
    amplitude.setTransport('beacon') 
    // Sets https transport to use `sendBeacon` API
    amplitude.flush()
  },
);

The readme for the amplitude-js-gtm library loaded/used by the GTM template shows the following docs for using setTransport via the GTM wrapper: https://www.npmjs.com/package/@amplitude/amplitude-js-gtm#settransport

window._amplitude(
  'setTransportType',
  'sendBeacon'
);

Looking at the underlying source repos, it seems there may be two typos here:

  1. setTransportType is not in the enum of available events (it appears it should be setTransport instead)
  2. in the underlying Browser 2.0 SDK, the browser-client's setTransport method expects an argument of type TransportType, which uses beacon (not sendBeacon):
export type TransportType = 'xhr' | 'beacon' | 'fetch';

This PR updates the docs to reflect the above, but it's entirely possible I'm just not following the source packages correctly...so please let me know if I'm entirely missing things! 🙈

Separately (and not implemented in this PR): would it be worth making the early return in the command wrapper throw an error or log when an unrecognized command is provided instead of silently ignoring it? https://github.com/amplitude/amplitude-js-gtm/blob/v3.x/src/amplitude-wrapper.js#L193

@dbertram
Copy link
Author

dbertram commented Nov 9, 2023

Actually, a colleague of mine, @carlosantoniodasilva, was doing some additional testing and it appears the documentation for using window._amplitude might be missing a first argument that's expected for all calls: the (optional) instance name.

The command wrapper appears to expect the first argument to be the amplitude instance name (or blank/undefined if you're only using a single instance):

// Pick the first argument as the instance name
var name = args.shift();
var client = !name ? globalAmplitude : globalAmplitude._iq[name];
if (!client) {
client = globalAmplitude.createInstance(name);
}
// Pick the first argument as the command
var cmd = args.shift();
// If cmd is not one of the available ones, return
if (eventEnum.indexOf(cmd) === -1) return;

As written, none of the calls in the README currently appear to function correctly since the first argument is shown as being the command name, but that's actually being shifted off the arguments list and used as an instance name.

In our testing, updating our code to include undefined as the first argument to window._amplitude does appear to get things working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant