Skip to content

Conversation

donaldpipowitch
Copy link
Contributor

@donaldpipowitch donaldpipowitch commented Dec 7, 2020

Summary

Fire callback, if user blocks Amplitude.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

Potentially? The behaviour changes to some degree, but it was always possible to call callback(0, 'No request sent', {reason: '...'}). So the user should be prepared for that.

@jooohhn jooohhn changed the title fixes #142 fix: logEvent callback on content block Dec 9, 2020
@jooohhn
Copy link
Contributor

jooohhn commented Dec 10, 2020

Hey @donaldpipowitch, thanks for this PR. Were you able this working on a test app?

I tried these changes out with create-react-app + ublock but the callbacks still weren't being called

import logo from './logo.svg';
import './App.css';
import amplitude from 'amplitude-js'
import React, { useEffect } from 'react';

const callback = (responseCode responseBody) => {
  console.log(responseCode)
  console.log(responseBody)
}

function App() {
  useEffect(() => {
    amplitude.getInstance().init("MY_API_KEY")
    amplitude.getInstance().logEvent('Amplitude-JavaScript-Demo: initialized', {}, callback)
  });

  return (
    <div className="App">
      <header className="App-header">
        <img src={logo} className="App-logo" alt="logo" />
        <button onClick={handleClick}>Send Event</button>
      </header>
    </div>
  );
}

export default App;

@donaldpipowitch
Copy link
Contributor Author

My bad... of course sending the request is still async. So this is triggered by _sendEventsIfReady which gets the callback even though _sendEventsIfReady doesn't expect a param. Strange.
But the callback is kept inside _unsentEvents and _sendEventsIfReady triggers sendEvents... which really makes a request - and there is no error handling. And it looks like I need to call removeEvents in order to invoke the previously saved callback. So I guess what I really need to fix is:

// utils.log('failed upload');

@jooohhn
Copy link
Contributor

jooohhn commented Jan 7, 2021

@donaldpipowitch I looked deeper into the fix. It belongs in _limitEventsQueued and I worked off your branch in #342. Thanks for bringing the issue up!

@jooohhn jooohhn closed this in #342 Jan 8, 2021
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.

2 participants