Legacy browser support: Issue with subclassing "Promise"

[rokoroku]

Related to #51.
react-async is hard to use in legacy ES5 browsers due to the implementation of NeverSettle class. The problem is subclassing native Promise is not supported in well-known transpilers, and to achieve this we have to polyfill Reflect and use custom transpilers.

References:

• babel: Problem with extending Promise babel/babel#1120
• typescript: Proposal: Add new __construct helper for better ES5/ES6 class interop microsoft/TypeScript#15397

Built-in classes such as Date, Array, DOM etc cannot be properly subclassed due to limitations in ES5 (for the transform-classes plugin). You can try to use babel-plugin-transform-builtin-extend based on Object.setPrototypeOf and Reflect.construct, but it also has some limitations.
https://babeljs.io/docs/en/caveats/#classes

Let me show an example:

• source:
  

  react-async/packages/react-async/src/reducer.js

  Lines 3 to 22 in ac09f5f

  	// This exists to make sure we don't hold any references to user-provided functions
  	class NeverSettle extends Promise {
  	constructor() {
  	super(() => {}, () => {})
  	/* istanbul ignore next */
  	if (Object.setPrototypeOf) {
  	// Not available in IE 10, but can be polyfilled
  	Object.setPrototypeOf(this, NeverSettle.prototype)
  	}
  	}
  	finally() {
  	return this
  	}
  	catch() {
  	return this
  	}
  	then() {
  	return this
  	}
  	}

• transpiled (ES5)

var NeverSettle = (function (_super) {
    __extends(NeverSettle, _super);
    function NeverSettle() {
        var _this = _super.call(this, function () { }, function () { }) || this;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                    ^ TypeError: undefined is not a promise

        if (Object.setPrototypeOf) {
            Object.setPrototypeOf(_this, NeverSettle.prototype);
        }
        return _this;
    }
    NeverSettle.prototype.finally = function () {
        return this;
    };
    NeverSettle.prototype.catch = function () {
        return this;
    };
    NeverSettle.prototype.then = function () {
        return this;
    };
    return NeverSettle;
}(Promise));

• simple reproducible code about the error:

Promise.call(this, function () {}, function () {})
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
^ TypeError: undefined is not a promise
    at Promise (<anonymous>)

So I wish we can change the NeverSettle implementation rather than force us to set it up that complex setup. For example:

class NeverSettle {
  constructor() {
    const promise = new Promise(() => {}, () => {});
    this.then = promise.then.bind(promise);
    this.catch = promise.catch.bind(promise);
    this.finally = promise.finally.bind(promise);
  }
}

[rokoroku]

My current workaround: Just replace the variable neverSettle as below.

const neverSettle = new Promise(() => {}, () => {});

The codebase doesn't use instanceof check, so it seems like it's okay to use like this way.

[ghengeveld]

Hey, thanks for reporting this! It's a pretty big problem so we need to find a proper solution. However, the new Promise workaround is not the right one. This was in fact the original implementation, but it was changed because it causes a memory leak, where callbacks chained on it will never be called and never garbage collected, because the promise itself never resolves. So instead, I'm looking to do something like this:

function NeverSettle() {}

Object.setPrototypeOf(NeverSettle, Promise)

NeverSettle.prototype = Object.assign(Object.create(Promise.prototype), {
  finally() {
    return this
  },
  catch() {
    return this
  },
  then() {
    return this
  },
})

[ghengeveld]

This is not the only place where we're extending a builtin class by the way. There's also FetchError.

Another thing to note is that Object.setPrototypeOf is not supported in IE 10, so we should check if this solution still holds there. I suspect it will not matter because we don't really care about the runtime type, only the TypeScript compile-time type.

[ghengeveld]

I've been trying to reproduce the transpilation failure using the Babel REPL, but haven't succeeded to trigger an error so far. What did you do to get these errors?

[ghengeveld]

Created a PR here: #175
So far only reimplemented NeverSettle. Still need to handle FetchError, but I want to be able to reproduce the transpile error before I continue.

[rokoroku]

I've been trying to reproduce the transpilation failure using the Babel REPL, but haven't succeeded to trigger an error so far. What did you do to get these errors?

I also cannot reproduce the issue on the Babel REPL. Maybe there's some patch after babel/babel#1120? (it's quite old issue 😅 and I just googled it to look into the situation.)

But it is possible on the Typescript Playground.

http://www.typescriptlang.org/play/?target=1#code/MYGwhgzhAEByCmA3eAnAyvALpk9rwA9N4A7AExgAUUB7AWwEsI8BvaAKGi+BpIkxQBXYJhooAFAEpobTly4RBAB1Tip0ALwA+GQF8ANNHXa90ufID0AKmhNMYEgCNBIWwHMSYvCUKZoViw55LgYAMyMAeUcAK3gRADpmTGoaUUwATxUI0OlZYMtA2FToMEQwBnBHXFsSaABJAFFoAEYABkNnP2AHaEc8JRoQdNCK3DIg-Ogo2ISklLTM+GzxTAALJkMEZHQsHHh4pVoFlTN83Qnoc-MRkjAQIfU84JQsQRRataYLq-luzGBVo8LlwXpg3h91jBzD8uGtSEDzCDXu9oJ8ofIfucgA

Still need to handle FetchError, but I want to be able to reproduce the transpile error before I continue.

Actually, subclassing "Error" doesn't make any problem in my experience so far. (and maybe that is "runtime error" rather than "transpile error"? 😅 )

But just in case, I leave a reference here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#ES5_Custom_Error_Object

[ghengeveld]

Can you verify that the PR I opened works? CodeSandbox automatically packs a release candidate: #175 (comment)

See https://ci.codesandbox.io/status/async-library/react-async/pr/175/builds/333
For usage instructions.

[rokoroku]

@ghengeveld I tested your PR in my codebase and now it works (without any runtime error) well :)

[ghengeveld]

Awesome. I'll ship it early next week, as I'm out for the weekend.