6

I want to extend Promise and change the then signature so its callback receives two values. I tried different approaches two of which are documented and tested here. Sadly, I get various errors or the resulting class does not behave like a Promise.

Approach 1: Wrapping a native Promise

export class MyWrappedPromise {
  constructor(data) {
    this.data = data;
    this.promise = new Promise(evaluate.bind(data));
  }

  then(callback) {
    this.promise.then(() => callback(this.data, ADDITIONAL_DATA));
  }

  catch(callback) {
    this.promise.catch(callback);
  }
}

Approach 2: Extending native Promises

export class MyExtendedPromise extends Promise {
    
  constructor(executor, data) {
    super(executor);
    this.data = data;
  }

  static create(data) {
      return new MyExtendedPromise(evaluate.bind(data), data);
  }

  then(callback) {
    return super.then(() => callback(this.data, ADDITIONAL_DATA));
  }
}

Does anyone have any suggestion on what I am doing wrong? Feel free to create a PR on GitHub.

thanks

------------------- Edit ---------------------

Some Additional code and info to make the code above more understandable without looking at the code and tests on Github.

evaluate is just the Promise executor function. I extracted it out so I can keep it consistent across all my implementations and tests. It may look convoluted but it's structured that way to simulate my "real" project.

export function evaluate(resolve, reject) {
  const data = this;
  function getPromise(data) {
    return !!data ? Promise.resolve(data) : Promise.reject(new Error("Error"));
  }

  getPromise(data)
    .then(resolve)
    .catch(reject);
}

ADDITIONAL_DATA is just a string to simulate the second value in the callback. It's also extracted to be consistent across all versions and tests.

------------------- Edit 2---------------------

Errors that come up depending on the solution

  • catch is not accessible
  • A lot of UnhandledPromiseRejectionWarning: warnings because errors/rejects are not getting propagated up correctly.
  • Errors/rejects are getting thrown too early and don't even reach the rejects checks in my test suites
Community
  • 1
  • 1
KenavR
  • 3,769
  • 9
  • 37
  • 47
  • use the wrapper instead. What error do you get? Where is `ADDITIONAL_DATA` defined and what is `evaluate.bind(data)`? – Nikos M. Dec 21 '18 at 10:48
  • Thanks for looking at it. I updated my initial post, they are just a `function` and `String` to keep it consistent across all my implementations and to better evaluate my test cases. – KenavR Dec 21 '18 at 10:57
  • what errors do you get? can you updfate your question? – Nikos M. Dec 21 '18 at 10:58
  • 1
    What's the goal behind this? – Daniel Hilgarth Dec 21 '18 at 11:05
  • I am trying to create a Promise like class that exposes `then` and `catch` but internally evaluates a promise based on the data passed to it. (`executor` is always the same). My main goal is to the callback of `then` receives two "data" parameters instead of one. **End Result** `ExtendedPromise.then((data1, data2) => ...).catch(error => ...)` – KenavR Dec 21 '18 at 11:12
  • "*I want to extend Promise and change the then signature so its callback receives two values.*" - that's a **really** bad idea. Don't do that. Just pass a tuple (an array of two values) to the callback and use destructuring syntax, like everyone else. – Bergi Dec 21 '18 at 11:22
  • Also, about your `evaluate` function: Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Dec 21 '18 at 11:24
  • 1
    I suggest not to use `extends Promise` but start with your own data structure. Make it [thenable](https://stackoverflow.com/a/29437927/1048572) to provide compatibility with promsises – Bergi Dec 21 '18 at 11:29
  • @Bergi I am sure you are right and I did quite a lot of things wrong with this question, but what I am trying to create is a class that "feels" like a promise for other people to use. It doesn't have to be an actual promise so changing the API is no big deal. It just would have been nice for people to use all the libraries (e.g. testing) around for Promises to basically by default work with my class. You are certainly correct about the anti-pattern, sorry I just hacked that stuff together late last night based on a way larger codebase. – KenavR Dec 21 '18 at 11:31
  • @KenavR What exactly do you mean by "feels like" if not offering the same API? But as I said, just make it thenable and all promise libraries will work with it. – Bergi Dec 21 '18 at 11:38
  • I want users to be able to use this syntax `ExtendedPromise.then((data1, data2) => ...).catch(error => ...)` The issues, depending on the implementation, I am having with the examples I posted above are 1) `catch` is not exposed, because the Promise is already resolved 2) Errors are not getting propagated correctly so `jest`'s `.rejects.toThrow(errorMsg)` does not detect the error correctly - even though it is thrown internally. 3) I get `UnhandledPromiseRejectionWarning:` possibly because of 2. – KenavR Dec 21 '18 at 11:43

2 Answers2

2

I don't understand very well why you do have a factory method, instead of using directly the constructor.

Do you mean something like this?

class MyExtendedPromise extends Promise {

  constructor(executor, data) {
    super(executor);
    this.data = data;
  }

  then(callback, test) {
    console.log('passed new parameter in then:', test);
    console.log('additional data:', this.data);
    return super.then(data => callback(data, test));
  }
}

new MyExtendedPromise((resolve, reject) => {
 setTimeout(() => resolve(true), 2000);
}, 'other additional data').then(data => console.log('my then', data), 'hello world');
quirimmo
  • 9,800
  • 3
  • 30
  • 45
  • Thanks for taking a look. I have the factory method because I would like to not pass in an executor function when creating the promise and rather just the data. The `executor` function is always the same. This scenario would probably make an `extend`ed version the wrong solution but I tried it to keep the resulting class as close to an actual Promise as possible. – KenavR Dec 21 '18 at 11:03
  • you mean that everytime you will create an instance of `MyExtendedPromise`, the executor will be always the same for every instance, and just the data passed in changes? – quirimmo Dec 21 '18 at 11:05
  • Exactly. It should be "chainable" and "catchable". – KenavR Dec 21 '18 at 11:08
  • so why you don't define your custom executor as method of the class and you call directly that code without passing in the executor? – quirimmo Dec 21 '18 at 11:09
  • That's what I am doing in the real version, but if you look at Github I have a couple of implementations and I use the same function for tests with an actual `Promise`, that's why I have extracted it out of the functions. My main problems are, that I either don't have access to `catch` or errors are not getting propagated up the chain correctly and I get a ton of `UnhandledPromiseRejectionWarning:` warnings. – KenavR Dec 21 '18 at 11:15
  • I now tried that version and added some tests and I have the same issue I have with my other solutions. `catch` is not exposed because the promise has already resolved. – KenavR Dec 21 '18 at 11:36
2

You have problems (especially with unhandled rejections) because you are not implementing the then interface correctly. Remember that .catch(onRejected) is just an alias for .then(undefined, onRejected), and then with two parameters is the actual core method of every promise.

You were always ignoring the second argument, so no rejection ever got handled. You need to write

then(onFulfilled, onRejected) {
  return super.then(res => onFulfilled(res, this.ADDITIONAL_DATA), onRejected);
  // or `this.promise.then` instead of `super.then`
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375