mozilla / testpilot-ga

Mozilla Public License 2.0
3 stars 9 forks source link

Check reject() usages and docs #16

Open pdehaan opened 6 years ago

pdehaan commented 6 years ago

Per https://github.com/mozilla/FirefoxColor/pull/234#pullrequestreview-112959286

I think the Promise resolve() and reject() methods only accept a single argument, but the code and README seem to have multiple arguments (so I think all additional arguments will be undefined). Per https://github.com/mozilla/testpilot-ga/blob/master/README.md step 3:

  .catch((response, err) => {
    console.error('Event failed while sending', response, err)
  });

Here's my super crude proof of concept:

function checkSuccess(name, value) {
  return new Promise((resolve) => {
    resolve(name, value);
  });
}

function checkFailure(name, value) {
  return new Promise((_, reject) => {
    reject(name, value);
  });
}

checkSuccess("peter", "pants")
  .then((name, value) => console.log("success!", "<<", name, value, ">>"));
// success! << peter undefined >>

checkFailure("peterf", "pantsf")
  .catch((name, value) => console.error("failure", "[[", name, value, "]]"));
// failure [[ peterf undefined ]]

If my stupid theory is correct, we may need to update the following code:

$ git grep "reject("

# These should probably only have a single argument. So not sure if we need to create a new Error
# instance above these references and inject whatever `req` data is possibly relevant.
src/TestPilotGA.js:          reject(req, Error(req.statusText));
src/TestPilotGA.js:        reject(req, Error("Network Error"));

# Probably don't need to be updated because they return a single value, but they _should_ 
# probably return an Error instance instead of a string value. But maybe `Promise#reject()`
# implicitly wraps it in an error when you throw a string.
src/TestPilotGA.js:        reject("Metrics not sent due to DNT.");
src/TestPilotGA.js:            reject(`Request error: ${req.statusText}`);
src/TestPilotGA.js:          reject(`Request error: ${req.status}`);
pdehaan commented 6 years ago

Looks like @lmorchard submitted a docs PR in #15, but the question still remains on if reject(req, Error(req.statusText)) and reject(req, Error("Network Error")) are doing what we expect.