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

ES6 compile target causes "Cannot call class as function" error #2

Closed
shrinktofit opened this issue Jan 9, 2019 · 20 comments
Closed
Labels
help wanted Extra attention is needed

Comments

@shrinktofit
Copy link

Our project use babel-typescript-plugin as compiler, in generated code, here is:

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

I see that ts-mixer call constructor explicitly.

See babel/babel#700 (comment)

@tannerntannern
Copy link
Owner

Is this a babel-specific issue? Does this cause errors for you when you compile or at runtime?

@tannerntannern tannerntannern added the question Further information is requested label Jan 9, 2019
@shrinktofit
Copy link
Author

yes it happen at runtime

@tannerntannern tannerntannern added the invalid This doesn't seem right label Jan 11, 2019
@tannerntannern
Copy link
Owner

I'll need more details in order to reproduce the issue. It sounds like it may be specific to babel, which makes me hesitant to investigate further. I'm closing this for now, but if you could reproduce the issue within the test suite I'd gladly look into it further.

@shrinktofit
Copy link
Author

It's easy to reproduce.

Just following code:

class Pen { public pen = 'string';  }
class Appe {  public appe = 'string'; }
class PPAP extends Mixin(Pen, Apple) {  }

const ppap = new PPAP();
console.log(ppap.pen);
console.log(ppap.apple);

with tsconfig.json:

{
  "compilerOptions": {
    "target": "es6",
    "module": "commonjs",
  }
}

Because ts will transform class direct as class if target is set to 'es6'.

@tannerntannern tannerntannern added bug Something isn't working and removed invalid This doesn't seem right question Further information is requested labels Jan 14, 2019
@tannerntannern tannerntannern changed the title Cannot call a class as a function ES6 compile target causes "Cannot call class as function" error Jan 14, 2019
@tannerntannern
Copy link
Owner

Thank you for adding more information. I now see the issue. I will have to think of the best way to resolve that.

@tannerntannern tannerntannern added the help wanted Extra attention is needed label Jan 14, 2019
@tannerntannern
Copy link
Owner

As far as I can tell, this unfortunate change from es5 to es6 is incompatible with the library. There's no way to call a constructor as a function without using new in es6, which is required for multiple inheritance to work at all. There are some proposals to allow constructors to be called as functions, but even that wouldn't solve the issue entirely.

Out of curiosity, is there any reason you must target es6? Targeting es5 doesn't cause these issues.

@shrinktofit
Copy link
Author

The reason is that our project had never limit our target to only ES5... :)

@tannerntannern tannerntannern removed the bug Something isn't working label Jan 14, 2019
@tannerntannern
Copy link
Owner

Well, the only suggestion I can give you is to not target ES6 for now. Calling constructor functions without new is what allows this library to work, so I'm afraid this is out of my control. I'm making a point to note this issue in the README.

I do appreciate the issue submission and I'm sorry there's not a better solution at the moment.

@danielpaull
Copy link

danielpaull commented Mar 29, 2019

I had a bit of a play with ES6 proxies. With some more work, perhaps this is a viable route to go for ES6?

function Mixin(...constructors: Constructor<any>[]) {	
  let MixinHandler = {      
    get(target : any[], propKey : string | symbol, receiver : any) {
        for( let b of target ) {
          if( Reflect.has( b, propKey ) ) {
            return Reflect.get( b, propKey, receiver )
          }
          
          // handle statics
          if( Reflect.has( b.constructor, propKey ) ) {
            return Reflect.get( b.constructor, propKey, receiver )
          }
          
        }
        return undefined;
    }
  }

  // define a new class with a proxy as its prototype that claims
  // to be a union of all the mixin types
  function X() {}
  X.prototype = new Proxy( constructors.map( c => new c ), MixinHandler );
  return (X as any)
}

@tannerntannern
Copy link
Owner

Thanks for looking into this! I have a few concerns though: Primarily, adding Proxies may work fine for ES6 compilation, but it will have to be polyfilled for earlier targets, which seems a bit overkill. Also, I'm not quite sure what purpose the Proxy serves here. Could you elaborate?

@danielpaull
Copy link

In ES6 you can't invoke the constructors as you've been doing (as is the subject of this issue). An alternative is to construct them in the normal manner and then compose them using a Proxy so that the proxy appears to have the union of all the properties of the classes being mixed in. The major difference will be the handling of "this", but there is probably some rebinding that could be done to set "this" to be the proxy (or the final class) if that is desirable.

In my code snippet, Mixin() returns a new class that has the proxy as it's prototype. This allows a class to derive from Mixin( A, B, ... ) just like your original implementation, making this a drop-in replacement that is TypeScript friendly.

There is no way to polyfill Proxy, so there would be two implementations, with the appropriate on selected at runtime.

@tannerntannern
Copy link
Owner

tannerntannern commented Apr 2, 2019

Thank you for the explanation. I think I understand what you're going for, but new Proxy( constructors.map( c => new c ), MixinHandler ); is still tripping me up. Doesn't this just create a proxy for the array of instantiated constructors? I am familiar with Proxies, and this doesn't seem like it would work as you describe. Perhaps there's a typo?

With that said, I might be able to be convinced to go down the Proxy route, but I'm still hesitant:

  • I'm not loving the idea of having parallel implementations of the Mixin function (even though it is small) and selecting the proper version at runtime.
  • As much as I love Proxies (I even have a published package related to Proxies), I'm not a fan of introducing them when users of the library might not expect them to be used under the hood. There could be a number of unexpected side effects.
  • I've also heard that Proxies can impact performance (which may be too negligible to quibble about, but it's worth noting nonetheless)

I'd much rather see an implementation that constructs the instances and manually merges them instead of using a Proxy and introducing more complexity and compatibility issues. But even then, while it may work for most use-cases, there are ways that usages of this in the constructors could cause unexpected problems. The only way to avoid those issues is to apply all the constructors to the same this, which can't be done without calling the constructor as a function. 😞

I don't want to shoot down your idea, just talking through my concerns.

@danielpaull
Copy link

Yes, the target of the Proxy is an array of objects. The traps (only partially implemented) on the Proxy search this array of objects for objects to as required.

Just a couple of comments on your three bullet points:

  1. Agreed completely. I think that this was a mistake in ES6. Breaking changes in the language should be replaced with an alternative. In this case they could have supplied a less convenient way to construct into an existing object.

  2. I'd be explicit about the implementation and runtime behaviour so users can make informed decisions. I think that the important thing is for the two implementation to have the same observable behaviour so users who upgrade from ES5 to ES6 are unaffected.

  1. Possibly, but proxies can hold state, so you could, for example, create an index over the properties of the composed parts to make property lookup faster than querying each part in turn.

I think that a library that requires users to target ES5 will become irrelevant over time (eg, I can't use it as I'm targeting ES6, but would have otherwise), so hopefully this idea provides at least one direction for you.

Best of luck - I'll leave this with you to mull over!

@tannerntannern
Copy link
Owner

Ah, I should have read the proxy handler more carefully. My apologies.

ES5 becoming irrelevant over time is definitely a compelling reason to reconsider this issue... Proxies may be the way to go.

On a different note, I've been pondering different ways one could "extract" a callable function given an ES6 class. Forgive my poor sketch (I'm low on sleep), but perhaps something like this could open new possibilities:

function classToFunction(clazz) {
    const source = clazz.toString();
    const constructorSource = /* use regex to extract body of constructor */;
    const constructorParams = /* use regex to extract constructor parameters */;

    return new Function(...constructorParams, constructorSource);
}

@danielpaull
Copy link

Something like that could work, but I'm sure that there would be some interesting cases to cover, like handling calls to super class constructors. Personally I feel like extracting code and then eval'ing should be a last resort, but if other options are exhausted then so be it.

Another option is to extend TypeScript through a Transformer and fiddle with the generated JS code that way. But, my limited experience with Transformers was not a particularly pleasant one (trying to get various webpack loaders to use the Transformer was arduous), but maybe things have improved. Ideally Microsoft would include a way to load transformers in the standard TypeScript compiler.

@tannerntannern
Copy link
Owner

I definitely agree that extracting and eval()ing code from a string feels very hackey. However, it avoids the compatibility issues previously discussed, and I think I have an idea on how to handle the super() situations. I'll be fiddling with it over the next few days. Feel free to peek at the progress on the 3.0-rewrite branch. Of course, if you or anyone else have better ideas, please don't hesitate to share!

@danielpaull
Copy link

Just thinking aloud - what happens with functions that are stringified and then eval'd in a different context, but they depend on variables defined in the original context, like imported modules or variables captured by a closure? Will this approach only work for a narrow set of use cases?

@tannerntannern
Copy link
Owner

Oh shoot, you're right... That might be impossible to get around too. The super call issue could be worked around because you can access the superclass constructor directly with Object.getPrototypeOf(SubClass.prototype).contructor, but closures are a whole different problem. I'll have to rethink my approach and/or give the Proxy option a closer look.

@tannerntannern
Copy link
Owner

tannerntannern commented Apr 18, 2019

Sorry for such a long delay. Just wanted to give a quick update:

I've exhausted several options to fix this issue, and for now the most practical, non-complicated solution is to simply use new and copy the properties over manually. Here's a snippet from the 3.0 branch that illustrates my simplistic approach:

// ...
class Mixed {
    constructor(...args) {
        for (const constructor of ingredients) {
            // If the constructor is a callable JS function, we would prefer to apply it directly to `this`,
            if (!isClass(constructor)) constructor.apply(this, args);

            // but if it's an ES6 class, we can't call it directly so we have to instantiate it and copy props
            else Object.assign(this, new constructor(...args));
        }
    }
}
// attach prototypes, static props, etc...

A bug in the TypeScript compiler is getting in the way of publishing a 3.0-rc at the moment, but otherwise, I should have something up in a few days, as long as no one spots any major issues with this approach.

@tannerntannern
Copy link
Owner

v3.0.0 has been released, which should resolve this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants