Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixed es5/es2015 code with mixin classes causes runtime errors #17088

Open
kitsonk opened this issue Jul 11, 2017 · 15 comments
Open

Mixed es5/es2015 code with mixin classes causes runtime errors #17088

kitsonk opened this issue Jul 11, 2017 · 15 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Jul 11, 2017

TypeScript Version: 2.4.1 (but likely anything since 2.2)

When utilising code targeted at ES5 mixed with code targeted at ES6 causes a runtime error when using mixin classes. The real world use case was using a library that was targeted at ES5 for distribution compatibility reasons while downstream code is being targeted at ES2017 because of running in a known limited environment.

Code

Tagged.ts

// @target: es5
export interface Constructor<T> {
    new(...args: any[]): T;
    prototype: T;
}

export default function Tagged<T extends Constructor<{}>>(Base: T) {
    return class extends Base {
        _tag = '';
    }
}

TaggedExample.ts

// @target: es2015
import Tagged from './Tagged';

class Example {
  log() {
    console.log('Hello World!');
  }
}

const TaggedExample = Tagged(Example);

const taggedExample = new TaggedExample(); // Uncaught TypeError: Class constructor Example cannot be invoked without 'new'

Expected behavior:

No run-time errors.

Actual behavior:

A runtime error of Uncaught TypeError: Class constructor Example cannot be invoked without 'new'.

@kitsonk
Copy link
Contributor Author

kitsonk commented Aug 1, 2017

I suspect this got lost in the shuffle. Ping @DanielRosenwasser

@kitsonk
Copy link
Contributor Author

kitsonk commented Aug 19, 2017

🎤 tap tap is this thing on? 😁

/cc @mhegazy

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 19, 2017

Hey @kitsonk, sorry we didn't catch this issue. I spoke about this with @rbuckton a quite a long time ago. Unfortunately I don't think that this is possible to achieve without Reflect.construct which isn't available in ES5 runtimes, but I will defer to Ron here.

@DanielRosenwasser DanielRosenwasser added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Aug 19, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Sep 5, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Sep 5, 2017
@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 5, 2017

@mhegazy the issue indicates that potentially a response from @rbuckton would help elucidate the issue. Can we keep it open or get a response?

@mhegazy mhegazy reopened this Sep 5, 2017
@rbuckton
Copy link
Member

rbuckton commented Sep 6, 2017

We would need to add a helper to facilitate marshalling super() to Reflect.construct when available and falling back to our existing behavior otherwise. Something like this:

var __construct = (this && this.__construct) || (typeof Reflect !== "undefined" && Reflect.construct
    ? function (self, target, args) { return target !== null && Reflect.construct(target, args, self.constructor) || self; }
    : function (self, target, args) { return target !== null && target.apply(self, args) || self; });

Which would then be used like this:

function Tagged(Base) {
    return (function (_super) {
        __extends(class_1, _super);
        function class_1() {
            var _this = __construct(this, _super, arguments);
            _this._tag = '';
            return _this;
        }
        return class_1;
    }(Base));
}

@mhegazy mhegazy added Suggestion An idea for TypeScript and removed Design Limitation Constraints of the existing architecture prevent this from being fixed labels Sep 6, 2017
@mgroenhoff
Copy link

We are currently running into this issue where we still emit most part of our codebase to es5 but only some are being migrated to es2015. Are there any plans to change the emitted es5 code like the example you posted? @rbuckton

@dpogue
Copy link

dpogue commented Jan 8, 2018

This continues to be an issue, making it impossible to subclass built-ins like Promises or EventTarget while targeting ES5 output

I've resorted to hand-transpiling some subclasses to use the __construct helper as posted above (and suggested in #15397), and it works as intended. Would be great to get this behaviour by default.

@unional
Copy link
Contributor

unional commented Apr 20, 2018

This is very problematic because it means my library (which rely on this) published in es5 cannot be consumed by JavaScript directly at all.

@RyanCavanaugh RyanCavanaugh added the Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature label Aug 15, 2018
@DanielRosenwasser DanielRosenwasser added Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this and removed Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels May 21, 2019
@fabiospampinato
Copy link

I just stumbled upon this. I'm not sure what more feedback is required, this bug basically breaks exporting class decorators in es5 and using them in es6+.

I had to resort to eval to work around this, check this if you're interested in the implementation.

@DanielRosenwasser
Copy link
Member

I think if someone wants to take a swing at an implementation, then we would be willing to discuss at a meeting and pull in the changes.

@DanielRosenwasser DanielRosenwasser added Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels May 21, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 16, 2019
@elado
Copy link

elado commented May 1, 2020

Looks like this works with Babel, because it creates a function around the super class (_createSuper helper):

"use strict";

// the untranspiled chunk
class A {}

// transpiled class B extends A {} with babel

function _instanceof(left, right) { if (right != null && typeof Symbol !== "undefined" && right[Symbol.hasInstance]) { return !!right[Symbol.hasInstance](left); } else { return left instanceof right; } }

function _typeof(obj) { "@babel/helpers - typeof"; if (typeof Symbol === "function" && typeof Symbol.iterator === "symbol") { _typeof = function _typeof(obj) { return typeof obj; }; } else { _typeof = function _typeof(obj) { return obj && typeof Symbol === "function" && obj.constructor === Symbol && obj !== Symbol.prototype ? "symbol" : typeof obj; }; } return _typeof(obj); }

function _classCallCheck(instance, Constructor) { if (!_instanceof(instance, Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function"); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, writable: true, configurable: true } }); if (superClass) _setPrototypeOf(subClass, superClass); }

function _setPrototypeOf(o, p) { _setPrototypeOf = Object.setPrototypeOf || function _setPrototypeOf(o, p) { o.__proto__ = p; return o; }; return _setPrototypeOf(o, p); }

function _createSuper(Derived) { var hasNativeReflectConstruct = _isNativeReflectConstruct(); return function () { var Super = _getPrototypeOf(Derived), result; if (hasNativeReflectConstruct) { var NewTarget = _getPrototypeOf(this).constructor; result = Reflect.construct(Super, arguments, NewTarget); } else { result = Super.apply(this, arguments); } return _possibleConstructorReturn(this, result); }; }

function _possibleConstructorReturn(self, call) { if (call && (_typeof(call) === "object" || typeof call === "function")) { return call; } return _assertThisInitialized(self); }

function _assertThisInitialized(self) { if (self === void 0) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return self; }

function _isNativeReflectConstruct() { if (typeof Reflect === "undefined" || !Reflect.construct) return false; if (Reflect.construct.sham) return false; if (typeof Proxy === "function") return true; try { Date.prototype.toString.call(Reflect.construct(Date, [], function () {})); return true; } catch (e) { return false; } }

function _getPrototypeOf(o) { _getPrototypeOf = Object.setPrototypeOf ? Object.getPrototypeOf : function _getPrototypeOf(o) { return o.__proto__ || Object.getPrototypeOf(o); }; return _getPrototypeOf(o); }

var B = /*#__PURE__*/function (_A) {
  _inherits(B, _A);

  var _super = _createSuper(B);

  function B() {
    _classCallCheck(this, B);

    return _super.apply(this, arguments);
  }

  return B;
}(A);

and new B() works.

TS uses A.apply(..) directly, which is invalid with native classes:

"use strict";

// the untranspiled chunk
class A {}

// transpiled class B extends A {} with TS

var __extends = (this && this.__extends) || (function () {
    var extendStatics = function (d, b) {
        extendStatics = Object.setPrototypeOf ||
            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
            function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
        return extendStatics(d, b);
    };
    return function (d, b) {
        extendStatics(d, b);
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
})();
var B = /** @class */ (function (_super) {
    __extends(B, _super);
    function B() {
        return _super !== null && _super.apply(this, arguments) || this;
    }
    return B;
}(A));


new B() // fails!

/*
Uncaught TypeError: Class constructor A cannot be invoked without 'new'
    at new B (<anonymous>:18:42)
    at <anonymous>:1:1
B @ VM714:18
(anonymous) @ VM738:1
*/

Because this fails:

class C {}
C.apply({}, []) // Uncaught TypeError: Class constructor C cannot be invoked without 'new'

@rbuckton
Copy link
Member

rbuckton commented Jan 5, 2023

I can't reproduce the error [...]

This issue still exists, but as more and more projects move to targeting ES2015 and higher I'm not sure it is a high enough priority to be fixed as the chances of running afoul of this are smaller and smaller. I expected the largest cause of this comes from extending builtins like Map and Set, but extending builtins seems to be something TC39 members are actively discouraging these days (with the possible exception of Error).

@DanielRosenwasser do we want to close this, or take a fix? I put together a fix for this, but it requires adding the __construct helper I mentioned above, which would change the emit for everyone using subclassing and --target ES5. It also would require a change to tslib, and that would require anyone affected by the change to upgrade to the latest tslib.

Alternatively, we could fix this behind a flag for the rare cases where this happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests