Skip to content

Make 'new.target' emit more precautions #15474

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

Open
rjamesnw opened this issue Apr 29, 2017 · 11 comments
Open

Make 'new.target' emit more precautions #15474

rjamesnw opened this issue Apr 29, 2017 · 11 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@rjamesnw
Copy link

rjamesnw commented Apr 29, 2017

I noticed that the polyfill for new.target ...

class Foo {
    constructor() {
        if (new.target)
            alert("Good.");
        else
            alert("Bad!");
    }
}

... outputs to this:

var Foo = (function () {
    function Foo() {
        var _newTarget = this.constructor;
        if (_newTarget)
            alert("Good.");
        else
            alert("Bad!");
    }
    return Foo;
}());

That seems dangerous to me, since this is always a true condition for older browsers that don't yet support new.target. Does it not make more sense to create an output such as this?:

var Foo = (function () {
    function Foo() {
        var _newTarget = this && this.constructor !== Window ? this.constructor : void 0;
                         /* (have to check 'this' also, in case of strict mode) */
        if (_newTarget)
            alert("Good.");
        else
            alert("Bad!");
    }
    return Foo;
}());

The following code fails to work as expected in Chrome v56.0.2924.87 (output is "Good." in call cases):

class $Foo {
    constructor() {
        if (new.target)
            alert("Good.");
        else
            alert("Bad!");
    }
}

type FooConstructor = typeof $Foo;

interface CallableFoo extends FooConstructor { (): $Foo; }

new $Foo(); // ok
var Foo: CallableFoo = <any>Foo;
Foo(); // ok? :/

@DanielRosenwasser DanielRosenwasser added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Design Limitation Constraints of the existing architecture prevent this from being fixed labels Apr 29, 2017
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 29, 2017

Technically we could do this - but I don't think it's worth it. TypeScript already gives you an error if you try calling your class (and in ES2015 it's an error to do so). Are people honestly going to be new.target and type asserting the class to any? Seems like you're really going out of the way to get the type-checker to come short.

@rjamesnw
Copy link
Author

rjamesnw commented Apr 30, 2017

Yes, from a business app perspective it might not make sense, but I'm trying to see if it is possible to use the constructor system to both construct and reconstruct an instance for better performance. This is a common practice for game development, where using "new" is never a good idea, but rather a disposal pattern of creating and rebuilding disposed types. You might think I could just use a "create" static function, but no, that doesn't work, since derived types could not create a new parameter signature like you can do with constructors, so I'm stuck with constructors in TS (especially if I want to use get/set properties on a class, which is not supported anywhere else).

See SO: http://stackoverflow.com/questions/18364175/best-practices-for-reducing-garbage-collector-activity-in-javascript (quote: "3 TravisJ - It's not at all uncommon in game frameworks.")

And here: http://buildnewgames.com/garbage-collector-friendly-code/#avoid-creating-objects

Add Construct 2 (most popular HTML5 game dev tool): https://www.scirra.com/blog/76/how-to-write-low-garbage-real-time-javascript (quote: "First of all, most obviously, the new keyword indicates an allocation, e.g. new Foo(). Where possible, try to create the object on startup, and simply re-use the same object for as long as possible.")

It doesn't break anything, and in fact makes it better, so I couldn't see why not? (nothing wrong with making things more reliable) :) I'd like to see Typescript better support the gaming community as well with better support for things like callable constructors (yes, I know there's already a long discussion on this #183), and so on, otherwise the workarounds make for very poor looking end user frameworks.

@DanielRosenwasser DanielRosenwasser added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Apr 30, 2017
@rjamesnw
Copy link
Author

rjamesnw commented Apr 30, 2017

Here is an example of what I mean: https://goo.gl/uf0teb

module Framework {

    export function registerType<TCallableType>(_type: { new (isnew: boolean, ...args: any[]): any }): TCallableType {
        // ... do magic here ...
        return null;
    }

    class $Foo {
        constructor(isnew: boolean) {
            if (isnew)
                alert("Brand new object.");
            else
                alert("Reinitialize object."); // (pulled from a cache)
        }
    }

    type FooType = typeof $Foo;
    export interface FooConstructor extends FooType { (): $Foo }
    export var Foo = registerType<FooConstructor>($Foo);
}

var foo = Framework.Foo();

class $Bar extends Framework.Foo {
    constructor(isnew: boolean) { super(isnew); }
}

type BarType = typeof $Bar;
interface BarConstructor extends BarType { (): $Bar }
 var Bar = Framework.registerType<BarConstructor>($Bar);

 var bar = Bar(); // (and also can inherit from 'Bar')

// ...etc... 

This is the closest I can get. The actual code is a bit more complex, but the jist of it is above. The registerType() function generates an intermediate factory function that calls the constructor on a cached item, or 'new's up another if needed. The objects enforce a disposal pattern. I would love to see better support for this structure. Note also that Foo and Bar can have any parameters they want, which is not possible with static functions when inherited.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 30, 2017

What it sounds like is that you are banking on the fact that TypeScript will allow your class to be callable without erroring. You should probably not be making that assumption. We've had people rely on using this before a call to super() before which is against the ES spec. When we added a check against this, many of those people got an error and decided to ignore it. When we actually tightened our behavior to be more spec-compliant, those people were broken at runtime as well as compile-time.

@rjamesnw
Copy link
Author

rjamesnw commented Apr 30, 2017

I can't see why this would break. The constructors are just a chain of calls. There's no harm in me creating an "isnew" parameter, then making sure users of a framework put all the "init" logic in the constructor. As far as inheritance goes, only constructors can have different parameters, so there's not much choice anyhow.

@rjamesnw
Copy link
Author

rjamesnw commented Apr 30, 2017

Ah, nevermind. I just found out with more poking around (you sort of alluded to it) that there are specialized functions in ES6, and this direction is bad. It fails in Chrome with Uncaught TypeError: Class constructor ... cannot be invoked without 'new'. I can see this failing if TS outputs code as per spec when targeting the newer versions. Guess I'll have to think of some other factory inheritance based pattern of some sort. :/ Oh well, thanks for the input.

@ghost
Copy link

ghost commented May 13, 2017

"new.target" has been addressed in a number of issues and closed off as a new ES feature that is now "supported" since TS 2.2. Rather than open yet another issue on new.target (and as this issue seems to be "in discussion") may I add that it is not apparent how this newly supported feature can be actually be used both write and consume a function supporting it in TypeScript.

The following code illustrates what I have been trying to do (with various attempts using interface definitions) but to no avail.

  // interface Foo {
  //   color: string;
  // }

  // interface FooConstructor {
  //   new (color: string): Foo;
  //   (color: string): Foo;
  // }

  function Foo(color: string) {
    if (new.target) {
      console.log('Foo() called via new');
      this.color = color;
    }

    else {
      console.log('Foo() called as normal function');
      return new Foo(color);
    }  
  }

  const foo2: Foo = new Foo('red'); // supposed to log "Foo() called via new" & return a Foo instance
  const foo1: Foo = Foo('red'); // supposed to log "Foo() called as normal function" & also return a Foo instance

Note that by uncommenting out interface Foo {..} and adding a this: Foo specification to the function declaration helps to mitigate a compile error with the line this.color = color; i.e. function Foo(this: Foo, color: string) { ... } but we're still stuck figuring out how to make it all play nicely with new invocation.

@ghost
Copy link

ghost commented May 13, 2017

Update .. resorting to an cast hack & const function assignment circumvents all compile errors though somehow doesn't feel like it's the right thing to do.

This code works as commented:

  interface Foo {
    color: string;
  }

  interface FooConstructor {
    new (color: string): Foo;
    (color: string): Foo;
  }

  const Foo: FooConstructor = <any> function(this: Foo, color: string) {
    if (new.target) {
      console.log('Foo() called via new');
      this.color = color;
    }

    else {
      console.log('Foo() called as normal function');
      return new Foo(color);
    }  
  }

  const foo2: Foo = new Foo('red'); // logs "Foo() called via new" & returns a Foo instance
  console.log('foo2', foo2);  // logs "foo2 _a { color: 'red' }"

  const foo1: Foo = Foo('green'); // logs "Foo() called as normal function" & "Foo() called via new" & returns a Foo instance
  console.log('foo1', foo1);  // logs "foo1 _a { color: 'green' }"

@rjamesnw
Copy link
Author

rjamesnw commented May 24, 2017

That would be fine for simple things, and was what I did originally. This does not work as static methods to a class to be inherited by other types - since classes cannot redefine the function call signature of static functions in the base type (which is where a "new" or similar modifier of sorts might come in handy). Ideally, it would be nice to call a simple and common NameSpace.MyObject.new() on all my types, which IS supported in ES6, but is a big limitation for TS (I should be allowed to redefine the static function on derived types, as anyone could normally in JS).

@rjamesnw
Copy link
Author

rjamesnw commented May 24, 2017

My example of a callable pattern, which better works with inheritance, is as follows:

function fromCacheOrNew<T extends { new (...args: any[]):any}>($type: T) : any {
    return <any>(() => {
        /* TODO: Check 'new.taret', get '$type' from cache, or ... */
        return new $type();
    });
}

// (file A)
module Types {
    class $A {
        x;
        static StaticVar = true;
    }
    $A['new'] = fromCacheOrNew($A);
    $A['$type'] = $A;

    interface IAFactory {
        'new'(s: string): $A;
        $type: typeof $A;
    }

    export var A: typeof $A & IAFactory = <any>$A;
}

// (file B)
module Types {
    class $B extends Types.A.$type {
        y;
    }
    $B['new'] = fromCacheOrNew($B);
    $B['$type'] = $B;

    interface IBFactory {
        'new'(b: boolean): $B;
        $type: typeof $B;
    }
    export var B:  typeof $B & IBFactory = <any>$B;
}

var a = Types.A.new("");
a.x = 1;

var b = Types.B.new(Types.B.StaticVar);
b.x = 1;
b.y = 2;


(https://goo.gl/zferqi)

Obviously, this is an idea only, and not complete. In the factory world of things, derived types would have to call the base types to initialize the instance if pulled from a cache (this pattern is common for JS games to reduce GC hits).

All this could easily be simplified if I could just force the change of inherited static member signatures.

@DanielRosenwasser DanielRosenwasser changed the title new.target Make 'new.target' emit more precautious May 24, 2017
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 24, 2017

Sorry, the "In Discussion" refers to the team discussing an amended new.target emit. The Discussion label (which is different, and has not been used on this issue) is used when we want to enable community members to engage.

Let's keep this issue open to people who need an amended new.target emit and not get side-tracked. 😄

@mhegazy mhegazy changed the title Make 'new.target' emit more precautious Make 'new.target' emit more precautions May 24, 2017
@RyanCavanaugh RyanCavanaugh removed the In Discussion Not yet reached consensus label Mar 8, 2019
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 Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants