Closed kevinresol closed 4 years ago
I used a similar strategy to https://github.com/kevinresol/hxgenjs/issues/42 to "fix" this. It's not applied to extern imports, but I don't think this is the case for you? Other than that some classes which would trigger a cyclic import are generated differently and I'm guessing that's where this can go wrong. Could you provide an error or minimal test case?
I'm playing with coconut and seems like no magic happened there?
Repro:
class Main extends coconut.ui.View {
static function main() {}
function render() '<div/>';
}
-lib coconut.react-dom
coconut.ui.View
in this case extends from coconut.ui.ViewBase
which extends NativeComponent
, which is an extern:
https://github.com/MVCoconut/coconut.react-core/blob/842457e7dfa0ced5a5c52e270a82d3f0dafb0660/src/coconut/ui/ViewBase.hx#L13
I believe this is not supported with -D js-es=6
either, https://github.com/HaxeFoundation/haxe/pull/7806 says:
There should be a compile-time error when we're trying to use super()-before-this or/and field inits when inheriting from as non-haxe-generated extern ES6 class.
In any case I'm wondering if genes should continue to export classes, these kind of annoyances might not be worth it.
argh.... perhaps @back2dos can help?
es6 is essential for tree-shaking to work
ES6 import/exports are for most tools but not the class syntax itself. Function constructor + prototype with a PURE
comment should work equally well for tree shaking. And it should take care of this issue as well as make generation easier
Ok, I got it to work with spherical chicken in a vacuum \o/
Here's a slightly more complex example:
@:explain class Main extends coconut.ui.View {
var foo = 123;
static function main() {}
function render() '<div />';
function viewDidMount()
trace(foo);
}
This yields the following:
public function new(__coco_data_:{ }):Void {
var snapshot = null;
var __afterRender = null;
super(render, null, null, null, function(firstTime:Bool) {
__afterRender(firstTime);
});
tink.macro.Bouncer.catchBounce(0);
tink.macro.Bouncer.catchBounce(1);
__afterRender = function(firstTime:Bool) {
if (firstTime) viewDidMount();
};
this.__initAttributes(__coco_data_);
switch (null) {
case null:
case v:{
beforeUnmounting(v);
};
};
}
And in JS it gives us:
constructor(__coco_data_) {
this.foo = 123;
var _gthis = this;
var snapshot = null;
var __afterRender = null;
super(this.coconutRender.bind(this), null, null, null, function (firstTime) {
__afterRender(firstTime);
});
this.__tink_defaults0 = {};
this.__slots = {};
__afterRender = function (firstTime1) {
if (firstTime1) {
_gthis.viewDidMount();
};
};
this.__initAttributes(__coco_data_);
}
As you see, there's two issues:
this.foo
is generated at the wrong position. It seems like it's somehow a side-effect of how this normally works:
class Base {
function new() {}
}
class Main extends Base {
var foo = 123;
function new() {
super();
}
static function main() {
trace(new Main().foo);
}
}
Becomes this:
export class Base {
constructor() {
this.new.apply(this, arguments)
}
new() {
}
}
export class Main extends Base {
new() {
this.foo = 123;
super.new();
}
static main() {
console.log("tests/Main.hx:10:",new Main().foo);
}
}
I wonder if it might be better to use ES6 class fields instead for constant field initialization?
var _gthis = this
captures this
too soon. I've added the indirection with __afterRender
so that the actual capturing occurs later, but the generated code looks to be the same. Again seems to be an artifact of how this is normally treated:
class Base {
function new() {}
}
class Main extends Base {
function new() {
super();
trace(function () trace(this));
}
static function main() {
trace(new Main());
}
}
And now we get:
export class Base {
constructor() {
this.new.apply(this, arguments)
}
new() {
}
}
export class Main extends Base {
new() {
var _gthis = this;
super.new();
console.log("tests/Main.hx:7:",function () {
console.log("tests/Main.hx:7:",_gthis);
});
}
static main() {
console.log("tests/Main.hx:10:",new Main());
}
}
There are probably ways this could be avoided. OTOH I wonder if the criterion for generating this super.new()
workaround could be widened to work in this case (i.e. if the base class is non-extern). In this case View
extends ViewBase
and both are not extern. It seems to me that ViewBase
could call super
and then this.new.apply(this, arguments)
. Or am I missing something?
As for the last comment: the body of the constructor which extends the extern class will have to call super
at some point (if extern defines a constructor). I guess what we could do is check the position of that super expression, and ignore it if in first position or error if it comes later.
Then again haxe generates stuff like var _gthis = this
before the super call, so that will make a position check impossible or very hacky.
In fact this.coconutRender.bind(this)
is also "before" super()
Hmm, in an ideal world we could avoid this-capturing with arrow functions ... but it looks like the typed AST is generally playing against us ^^
Looked into this a little more. The reason why we can call super
after accessing this
in regular haxe generation is simply because it generates the super call as ExternClass.call(this, ...arguments)
. Because almost all node modules will ship with code transpiled to ES5, where constructor is a regular function, makes it work most of the time. I guess it's a better default than trying to call super
where it might fail in all but the simplest scenarios (with haxe's generation randomly accessing this
before super
even if you're careful not to). To do so however we'll need our own constructor to be a regular function, not a class constructor, because that would require it to call super
which we don't want to - it should be called as a function in the new
method.
Gave it a go in 66bceb5dc275881af8d7c87eadd695a76478a625, could you test your scenario?
A very rough try seems to show that it is incompatible with React.createElement()
, because it expects a class with constructor.
Class constructor App cannot be invoked without 'new'
Are you using a bundler on the output?
What happens here is React does a check to see if it should construct a component as class
, with new
, or as a function
:
https://github.com/facebook/react/blob/e706721490e50d0bd6af2cd933dbf857fd8b61ed/packages/react-reconciler/src/ReactFiber.js#L370
Since we don't directly extend the React.Component
prototype (that happens upon construction) React tries to instantiate the component as a function which fails. Under development it prints some warning too:
Warning: The <MyComponent /> component appears to have a render method, but doesn't extend React.Component. This is likely to cause
errors. Change MyComponent to extend React.Component instead.
I managed to reproduce but also successfully render if I fake this property on the prototype: https://github.com/benmerckx/genes/blob/master/tests/TestReactComponent.hx#L40-L42
We might look for a way to realiably fake - because the way Haxe modules are structured we need to defer resolving the inheritance chain until creation in order to support ES6 modules with circular imports.
Got it to pass without any tricks by directly extending the super prototype when possible. Checking for circular imports for every super class slows down compilation but I guess I could look into making that a little more performant with a graph. You could give it another try if you're still working on this :)
I am getting these strange imports deb9e869fdf24f6de5723a18563a20925d9896ee
import React from "react"
import {Component as React.Component, Fragment as React.Fragment} from "react"
Okay, guess I was too fast :) Should be okay with 5dac6e38e3cb60a0c5f58eea7edf5758c97cbba4
Yeah seems working now. Although apparently I am getting some problems related to non-default imports or wildcard imports, will investigate and file them later when I have time. (Happy Chinese New Year!)
I'll close this one. Someday I might look into making this configurable (to enable extending actual runtime classes)
I think the vanilla Haxe generator has already fixed that (with `-D js-es=6), perhaps could be ported here?