[js] support creating extern classes by calling a constructor function without "new" keyword

[nadako]

See #2784 (comment)

It seems that it's quite common in JS libraries that objects are created by simply calling a function instead of using "new SomeConstructor" syntax (e.g. JQuery, express, socket.io), so I think it makes sense for us to support that.

The most straightforward way is probably to use metadata for the constructor field that simply makes JS generator omit the new keyword, i.e.

extern Express {
    @:noNew // we need a better name for it
    function new();
}

But I don't really know to what extent is it compatible with e.g. Type.createInstance. @aduros, @clemos, @eduardo-costa any comments?

[eduardo-costa]

Let's see.
I'm not against "noNew" or something "ugly" but clear.

Some suggestions.
extern Express
{
@:method
@:direct
@:noOp
function new();
}

Regarding the "Type" functionalities I don't know how it would reflect on
the current implementation.
The ideal scenario is that Type.createInstance keep working.

2014-10-05 19:10 GMT-03:00 Dan Korostelev [email protected]:

See #2784 (comment)
#2784 (comment)

It seems that it's quite common in JS libraries that objects are created
by simply calling a function instead of using "new SomeConstructor" syntax
(e.g. JQuery, express, socket.io), so I think it makes sense for us to
support that.

The most straightforward way is probably to use metadata for the
constructor field that simply makes JS generator omit the new keyword,
i.e.

extern Express {
@:noNew // we need a better name for it
function new();}

But I don't really know to what extent is it compatible with e.g.
Type.createInstance. @aduros https://github.com/aduros, @clemos
https://github.com/clemos, @eduardo-costa
https://github.com/eduardo-costa any comments?

—
Reply to this email directly or view it on GitHub
#3441.

[image: Imagem inline 1]

[nadako]

IIRC, calling "new" in js creates an empty object, calls the constructor function with this set to that new object and then returns that object. But it's not clear to me how express and others work. We have to do some research.

[eduardo-costa]

​I think that independently of the inner​ workings the result of:

var myref = new Class()
var myref = ClassCreator();

Is the same.
The returning of an instance.
Some cases can be the same instance (JQuery I guess) in others a new
instance.
But what the user wants is to have the completion of the desired class.
It is up to him to know when that operation is returning something new or a
re-used instance.

2014-10-05 19:28 GMT-03:00 Dan Korostelev [email protected]:

IIRC, calling "new" in js creates an empty object, calls the constructor
function with this set to that new object and then returns that object.
But it's not clear to me how express and others work. We have to do some
research.

—
Reply to this email directly or view it on GitHub
#3441 (comment)
.

[image: Imagem inline 1]

[nadako]

Yeah, but can we safely use Type.createInstance which itself uses new Class under the hood?

[eduardo-costa]

We should adapt it some way.
But it uses the "new" that is native of JS or it calls the "new" method of
the Haxe class?

2014-10-05 19:45 GMT-03:00 Dan Korostelev [email protected]:

Yeah, but can we safely use Type.createInstance which itself uses new
Class under the hood?

—
Reply to this email directly or view it on GitHub
#3441 (comment)
.

[image: Imagem inline 1]

[ncannasse]

Is it a coding style difference or is there an actual runtime difference between the two ?

[nadako]

I'm not sure. AFAIK, in JS if a constructor function returns an object, it will be used as a value of new expression instead of created empty object. So with regard to runtime difference, the only concern is that a redundant empty this object is created and not used. However I believe this is a case modern JS engines do optimize. Correct me if I'm wrong.

[clemos]

I believe they are quite equivalent.
But I read about it yesterday, and it seems new cl() is slightly slower than cl().
I still don't think it's really a major issue, so I'm in favor of keeping new for now.

[Simn]

I don't really see the point of this either. Unless someone proves that there's a noticeable runtime difference I'm in favor of keeping the status quo.

[eduardo-costa]

The problem still remains.
Some instances are created without "new".
Like "Application" in the express lib.
Making it hard to do it using only externs.
Em 06/10/2014 10:09, "Simon Krajewski" [email protected] escreveu:

Closed #3441 #3441.

—
Reply to this email directly or view it on GitHub
#3441 (comment).

[Simn]

I might be misunderstanding the problem here.

[eduardo-costa]

Actually for this issue to be solved, HaxeJS as a whole needs this feature.

JQuery, Express and others libs can have this "new-less" instantiation.
Em 06/10/2014 10:52, "Simon Krajewski" [email protected] escreveu:

Reopened #3441 #3441.

—
Reply to this email directly or view it on GitHub
#3441 (comment).

[Simn]

I thought this was optional and you might as well use new.

[clemos]

Simn is right.
Doing var foo = new JQuery('.foo') and var foo = JQuery('foo') are pretty much exactly the same in JS, so this is at most an optimization.

[nadako]

Yeah you can do new Applicationas well as just Application, it should be the same.

[aduros]

They are not the same, but some library implementors write their constructors to be callable both ways: http://elegantcode.com/2010/12/21/basic-javascript-part-4-enforcing-new-on-constructor-functions/

I think this safety convention was started back in JQuery, but many (most?) modern libs these days don't bother.

[nadako]

I don't think those libraries both work with this and return an object in their constructor functions at the same time, so it should be safe in 99% cases.

[eduardo-costa]

Still I think the "noNew" meta is still necessary.

2014-10-06 11:23 GMT-03:00 Dan Korostelev [email protected]:

I don't think those libraries both work with this and return an object in
their constructor functions at the same time, so it should be safe in 99%
cases.

—
Reply to this email directly or view it on GitHub
#3441 (comment)
.

[image: Imagem inline 1]

[hexonaut]

Calling with and without new are not the same. However, we are only interested in the case where a constructor function is called with new when it is supposed to be called without. In this case the object the function is returning must be the newly instantiated object, and according to new symantics in JavaScript: the returned object will be used instead of the newly created object by the new keyword. So effectively this will be the same result.

Unless I am missing something this means we do not need a "noNew" meta, unless we are concerned about the extra object that gets allocated.

[eduardo-costa]

But the user will need something like this if he wants to use Express.

//Must be compiled to:
//var app = require('express')();
var app : Application = new Application();

2014-10-06 13:05 GMT-03:00 Sam [email protected]:

Calling with and without new are not the same. However, we are only
interested in the case where a constructor function is called with new when
it is supposed to be called without. In this case the object the function
is returning must be the newly instantiated object, and according to new
symantics in JavaScript: the returned object will be used instead of the
newly object created by the new keyword. So effectively this will be the
same result.

Unless I am missing something this means we do not need a "noNew" meta,
unless we are concerned about the extra object that gets allocated.

—
Reply to this email directly or view it on GitHub
#3441 (comment)
.

[image: Imagem inline 1]

[hexonaut]

What's wrong with var app = new require('express')(); ?

Will give the same result.

[eduardo-costa]

I tought it wouldn't work.
If it works I'm ok with it.

2014-10-06 13:16 GMT-03:00 Sam [email protected]:

What's wrong with var app = new require('express')();

Will give the same result.

—
Reply to this email directly or view it on GitHub
#3441 (comment)
.

[image: Imagem inline 1]

[fponticelli]

It doesn't work in all instances ... sometimes you need to first assign the
"type" to a variable. I will try to find a case where that doesn't work but
it is quite common.

On Mon, Oct 6, 2014 at 10:21 AM, Eduardo Dias da Costa <
[email protected]> wrote:

I tought it wouldn't work.
If it works I'm ok with it.

2014-10-06 13:16 GMT-03:00 Sam [email protected]:

What's wrong with var app = new require('express')();

Will give the same result.

—
Reply to this email directly or view it on GitHub
<
https://github.com/HaxeFoundation/haxe/issues/3441#issuecomment-58042560>
.

[image: Imagem inline 1]

—
Reply to this email directly or view it on GitHub
#3441 (comment)
.

[eduardo-costa]

Yes sometimes it returns the "Class"

var TheClass = require("theclass");
var ref = new TheClass();

or it can be the instance.

var ref = require("express")();

2014-10-06 13:38 GMT-03:00 Franco Ponticelli [email protected]:

It doesn't work in all instances ... sometimes you need to first assign
the
"type" to a variable. I will try to find a case where that doesn't work
but
it is quite common.

On Mon, Oct 6, 2014 at 10:21 AM, Eduardo Dias da Costa <
[email protected]> wrote:

I tought it wouldn't work.
If it works I'm ok with it.

2014-10-06 13:16 GMT-03:00 Sam [email protected]:

What's wrong with var app = new require('express')();

Will give the same result.

—
Reply to this email directly or view it on GitHub
<
https://github.com/HaxeFoundation/haxe/issues/3441#issuecomment-58042560>

.

[image: Imagem inline 1]

—
Reply to this email directly or view it on GitHub
<
https://github.com/HaxeFoundation/haxe/issues/3441#issuecomment-58043297>
.

—
Reply to this email directly or view it on GitHub
#3441 (comment)
.

[image: Imagem inline 1]

[hexonaut]

What's wrong with

var TheClass = require("theclass");
var ref = new TheClass();

being

var ref = new require("theclass")();

?

[eduardo-costa]

This case is ok.
But there is the expressjs case

//CTOR for new Application in the express lib.
var app = require("express")(); //not using 'new'

[frabbit]

with intrusive you mean to expensive?

[frabbit]

maybe it could only be checked if the class is extern?

[Simn]

I don't think it would be significant, but we would have to add it in quite a few places. The same issue could crop up with overrides and I don't even want to start thinking about what happens when constrained type parameters get involved.

I'm actually considering removing this feature entirely because it's so fragile. It might be better to apply native field names really early and somehow deal with field access, but that's just hacky as well.

[frabbit]

As a first step couldn't we just disallow unification of fields with @:native metadata with other types in general. Even if they have the same @:native metadata?

[clemos]

For the record, I have a macro that brings support of @:native for fields (currently vars only) through abstracts.
https://github.com/clemos/haxe-js-kit/blob/master/util/NativeMap.hx
Example: https://github.com/clemos/haxe-js-kit/blob/master/js/atomshell/browser/BrowserWindow.hx#L15
It's certainly too much of a hack, and I haven't been testing it a lot (the unification cases you're refering to), but it exists ;)

[frabbit]

@nadako I run into the selfCall problem over and over again when writing externs for gulp plugins, especially because they overload the function a lot. It would be very very helpful to have something like @:selfCall, because it would make the externs so much cleaner.

[frabbit]

btw. i used a valueOf hack before you dissallowed non valid js identifiers. But this doesn't work anymore ;)

one example:

@:jsRequire("gulp-watch") extern class GulpWatch
{
    @:overload(function (glob:String, ?options:GulpWatchOptions, ?cb:Stream->?(Void->Void)->Stream):Stream {})
    @:native("valueOf()") public static function watch (glob:String, ?options:GulpWatchOptions, ?cb:Stream->?(Void->Void)->Void):Stream;
}

how would you write it without something like @:selfCall

[frabbit]

I would argue that this:

@:jsRequire("gulp-watch") extern class GulpWatch
{
    @:overload(function (glob:String, ?options:GulpWatchOptions, ?cb:Stream->?(Void->Void)->Stream):Stream {})
    @:selfCall public static function watch (glob:String, ?options:GulpWatchOptions, ?cb:Stream->?(Void->Void)->Void):Stream;
}

looks very clean compared to another solution.

[frabbit]

i know that you could use EitherType in this example, but that's not an option when you have a different number of arguments.

[nadako]

Try now :-)

[frabbit]

wow that's nice, thx a lot. I spotted one last problem with it. If you assign a function to a variable this is not working:

var f = gulp.GulpWatch.watch; // generates var f = gulp.GulpWatch.watch; in js

[frabbit]

is this a problem with @:native too?

[frabbit]

should be this line right?

haxe/genjs.ml

Line 526 in bae7da5

| TField (x,f) ->

[nadako]

Yeah, i changed that in the gen_call, but it's probably more correct to handle it in TField case.

[nadako]

is this a problem with @:native too?

what's the problem?

[frabbit]

no @:native works as expected, it was just a question without testing ;)

[frabbit]

thx

[frabbit]

so much better now ;)

[Simn]

I lost track of what this issue is about. Can we just close it now? :)

[nadako]

I'm not sure, we're originally talked about self-calling constructors while I added a @:selfCall for fields, so maybe we should add support for TNew as well.

[Simn]

Ok, I'll just assign it to you then!

[clemos]

I was checking LuaXe, which has support for this feature (@nonew, through a custom generator...),
and was wondering if it was possible to simply generate something like that using a macro :

public inline function new( arg ) {
   return untyped __js__('MyClass')( arg );
}

[nadako]

okay, now genjs supports @:selfCall for both fields and constructor which allows one to write stuff like:

@:jsRequire("myapp")
extern class MyApp {
    @:selfCall function new();
    @:selfCall function run():Void;
}

class Main {
    static function main() {
        var app = new MyApp();
        app.run();
    }
}

generates:

(function () { "use strict";
var MyApp = require("myapp");
var Main = function() { };
Main.main = function() {
    var app = MyApp();
    app();
};
Main.main();
})();

@clemos @frabbit or anyone who is working on externs please confirm that it's good enough.