apache/flagon-useralejs

NPM deployment automatically starts regardless of usage

confusingstraw opened this issue · 15 comments

confusingstraw commented

The flagon-userale NPM package has a handful of issues. The most egregious (in my eyes) is that, even if I don't explicitly start it, it will begin collecting/sending logs as soon as it is imported. Maybe there is some argument that this is intended behavior, but having it do this is probably a surprise for developers. For example, this React project results in log collection/sending:

// in index.js
import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';

ReactDOM.render(
  <React.StrictMode>
    <App />
  </React.StrictMode>,
  document.getElementById('root')
);
// in App.jsx
import { packageCustomLog } from "flagon-userale";
import { useCallback, useEffect, useState } from "react";

function App() {
  const [secondsElapsed, setElapsed] = useState(0);
  const resetTimer = useCallback(() => {
    setElapsed(0);
    packageCustomLog({ elapsed: secondsElapsed, logType: "timerReset" }, undefined, false);
  }, [secondsElapsed, setElapsed]);

  useEffect(() => {
    const timerId = setTimeout(() => {
      setElapsed(secondsElapsed + 1);
      packageCustomLog({ elapsed: secondsElapsed, logType: "timerUpdate" }, undefined, false);
    }, 1000);

    return () => {
      clearTimeout(timerId);
    };
  }, [secondsElapsed, setElapsed]);

  return (
    <main className="App">
      <p>App has been running for {secondsElapsed} seconds.</p>
      <button onClick={resetTimer}>Reset</button>
    </main>
  );
}

export default App;

This surprises me, since it means that importing any part of userale will automatically start the logging process. I'd imagine we ought to figure out a way to have it not start by default when imported via NPM. Of course, this is a breaking change.

poorejc commented

That's a good ticket. So far haven't heard much concern from the users I'm connected with, but they were expecting this behavior. Maybe should add something to './src/getInitialSettings' and some examples for triggering UserALE's start state as well as clearly defined default start behavior and more "customized" options... What do you think "most developers" will expect from a package like this? This behavior is identical to the script tag option. I imagine there would be some level of refactoring needed to configure the exports such that if you just wanted to use packageCustomLogs, you're only getting that export and related exports/imports.

Likely big reconfiguring of './src/main.js'. Currently, all the libraries in ./src are effectively daisy-chained because we originally concieved of this as a script tag. Not a module with optional imports.

UncleGedd commented

Regarding developer expectations, I would not expect it to start logging everthing immediately. I'd also like to see this behavior turned off by default

confusingstraw commented

I imagine there would be some level of refactoring needed to configure the exports such that if you just wanted to use packageCustomLogs, you're only getting that export and related exports/imports.

that is correct, we don't actually import from src at all anyways. we have the main in package.json set to the built script, meaning it is an all-or-nothing deal. i think it may just be surprising, as there is a start function exported, but userale will actually start whenever some part of it gets imported somewhere. it is then the user's responsibility to call stop and then call options (to actually configure it), and finally call start to actually begin. all of this, mind you, also means they can't actually use the resolution option in the config, since it is applied during attachHandlers, which only gets called once (during initial startup).

poorejc commented

Cool. I can play around with this. Good ticket.

kevbrowndev commented

This has become an issue for us. Has anyone started work on this? If not I'll have a crack at it.

UncleGedd commented

@kevbrowndev good to hear from you! Happy to take a look at this. Will update by the end of the week

UncleGedd commented

@kevbrowndev I've started work on a proof-of-concept for this. In the meantime, a workaround for users who don't want userale to start automatically could be to call the stop function as mentioned above

kevbrowndev commented
UncleGedd commented

Ok, I may have stumbled into a super simple solution here. In getInitialSettings.js we set:

    settings.autostart = get('data-autostart') === 'false' ? false : true;

This causes userale to start up automatically unless the data-autostart property is explicity set to false. Which in the case of an NPM package, will not be set at all. This can be averted by simply inverting the logic to:

    settings.autostart = get('data-autostart') === 'true' ? true : false;

With this change, in an NPM deployment, the start function must be called before userale will start working, and in the case of a script deployment, users must set data-autostart to true. This is of course a breaking change for script users

kevbrowndev commented
confusingstraw commented

With this change, in an NPM deployment, the start function must be called before userale will start working, and in the case of a script deployment, users must set data-autostart to true. This is of course a breaking change for script users

Instead of making a breaking change for script users, is there perhaps a way to detect whether we are in a script vs NPM deployment? Maybe we can make use of available environment variables? (though that implies a build process)

kevbrowndev commented
UncleGedd commented

Yes, thank you, I noticed this too! I was wondering if there was some reason for it. How to properly test this fix? Do you think it would be a good idea to add a second, separate journey test, that tested autostart = false?

Adding a journey test and asserting if a network request did not occur would likely add signficant time to our test suite (waiting for Cypress to time out). I would try a unit test approach first, or just make the change and edit the unit/journey tests that would undoubtedly break

UncleGedd commented

In order to prevent breaking changes, what about this? In Userale, the configure() function could be altered at line 32 so that if autostart is set via the options() function, as could happened if userale is being used as an npm package, that value trumps the getInitialSettings() version of autostart. This way, the code should work the same as it does now, unless autostart value is set to true in the Userale.options() function. In that case, no Userale logs would be sent. https://github.com/apache/incubator-flagon-useralejs/blob/master/src/configure.js

I can't quite picture it, but I'm more than happy to take a look at a PR (a WIP is totally fine).

My bigger concern is around creating a userale behavior dichotomy between script vs NPM users. My opinion is that we should have Userale behave the same regardless of source. I'm defintely open to other opinions though

kevbrowndev commented

What about something like this?
Script tag Userale and NPM package Userale behave the same. Might be a tad kludgey...