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

Class support should include new.target #1088

Closed
getify opened this issue Mar 26, 2015 · 18 comments
Closed

Class support should include new.target #1088

getify opened this issue Mar 26, 2015 · 18 comments
Labels
Has PR help wanted outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Milestone

Comments

@getify
Copy link

getify commented Mar 26, 2015

Expected:

class Foo { constructor() { console.log(new.target.name); } }
var x = new Foo(); // "Foo"

and...

class Bar extends Foo { constructor() { super() } }
var y = new Bar(); // "Bar"
@sebmck
Copy link
Contributor

sebmck commented Mar 26, 2015

Yes, this is currently waiting on estree/estree#32 😜

@sebmck sebmck added enhancement PR: Spec Compliance 👓 A type of pull request used for our changelog categories labels Mar 26, 2015
@RReverser
Copy link
Member

This is already merged and available in Acorn.

@sebmck
Copy link
Contributor

sebmck commented Apr 13, 2015

From the looks of it you can just literally turn:

class Foo { constructor() { console.log(new.target.name); } }
var x = new Foo();

into:

class Foo { constructor() { console.log(this.constructor.name); } }
var x = new Foo();

right?

@getify
Copy link
Author

getify commented Apr 13, 2015

I think it may be more nuanced than that: https://github.com/getify/You-Dont-Know-JS/blob/master/es6%20&%20beyond/ch3.md#newtarget

In particular, in normal functions called without new and in all class methods, it has to be null.

@sebmck
Copy link
Contributor

sebmck commented Apr 13, 2015

Class methods and normal functions can be easily detected and alternate transformations done.

@jayphelps
Copy link
Contributor

Just to clarify, if not called via new, new.target === undefined, not null.

References: 1, 2

@monolithed
Copy link

Oh, I was hoping that it will be possible to do something like:

class Foo { 
   toString () {
     return `[object ${new.target.name}];`
   }
}

@sebmck
Copy link
Contributor

sebmck commented Apr 19, 2015

@monolithed For the time being you can just do this.constructor.name which will likely cover your use case completely.

@monolithed
Copy link

@sebmck, You're right, but the constructor may be redefined.

function Foo () {}

Foo.prototype = {
   constructor: function Bar () {},

   toString: function () {
      return this.constructor.name;
   }
}

new Foo() + ''; // Bar

As far as I understand, it is not possible with new.target.name

@RReverser
Copy link
Member

just my 5c.: new.target can be also overridden using Reflect.construct(), so doesn't make big difference.

@babel-bot
Copy link
Collaborator

Comment originally made by @thejameskyle on 2015-11-20T19:30:26.000Z

Marking this as low priority for now.


Comment originally made by @bergus on 2016-08-26T00:51:46.000Z

I think you should raise the priority of this issue. new.target is not transpiled at all currently, which does raise a syntax error in ES5.

See also this StackOverflow question. It seems like it was supported in Babel 5, is that correct?

andrewmunsell added a commit to andrewmunsell/themeparks that referenced this issue Dec 31, 2016
"new.target" is used to ensure that the base class cannot be instantiated directly, but this is an ES6 feature new to Node.js v5.0.0 that cannot be transformed directly by Babel v6 (babel/babel#1088) and results in a syntax error in versions of Node.js <5.0.0. This change fixes the issue using this.constructor and ensures that the script properly runs on older versions of Node.js when Babel is used to build the dist files.

This also adjusts the Travis CI Node.JS versions to include Node.JS 4 and 0.12, which are listed as supported in the package.json file.

Fixes cubehouse#45
cubehouse pushed a commit to cubehouse/themeparks that referenced this issue Dec 31, 2016
"new.target" is used to ensure that the base class cannot be instantiated directly, but this is an ES6 feature new to Node.js v5.0.0 that cannot be transformed directly by Babel v6 (babel/babel#1088) and results in a syntax error in versions of Node.js <5.0.0. This change fixes the issue using this.constructor and ensures that the script properly runs on older versions of Node.js when Babel is used to build the dist files.

This also adjusts the Travis CI Node.JS versions to include Node.JS 4 and 0.12, which are listed as supported in the package.json file.

Fixes #45
@hzoo hzoo added this to the Babel 7 milestone Apr 28, 2017
@stevenvachon
Copy link

2 years and nothing.

@stevenvachon
Copy link

stevenvachon commented Jun 29, 2017

from https://stackoverflow.com/a/33361029/923745:

if (!(this instanceof Foo)) {}

or:

You can rewrite an abstract class constructor like if (this.constructor.name === MyAbstractClass.name) {throw new Error("Cannot instantiate abstract class.");} It works great when extending ES6 classes with an abstract class. I had to do this since Babel still doesn't polyfill new.target so my Webpack UglifyJS plugin was reading it as a syntax error, breaking my production builds.

@hzoo
Copy link
Member

hzoo commented Jun 29, 2017

@stevenvachon, it's open for someone to take on if they want. Priority isn't determined based on the length of time an issue has been open either (no other signal). It's open source 🙂 , someone can make a PR

@loganfsmyth
Copy link
Member

If an issue hasn't been done, it's because our priorities are focused elsewhere. If someone volunteers to submit a PR we'd happily review it, but no-one has.

If you want to have a productive discussion about this feature, by all means, but I'm not sure what else to say other than that attempting to shame the project maintainers for not having your priorities isn't cool. It is also extremely demotivating, so it may very well have the opposite effect of what you're looking for.

@stevenvachon
Copy link

I'll try to find time, but I'm doubtful as I'm focused on other public projects.

@getify
Copy link
Author

getify commented Jun 29, 2017

@loganfsmyth I agree with your sentiment, and I agree that "shaming" is a really bad and demotivating pursuit in OSS.

But since we're on the topic, I'm curious: how should non-project members (of any project) bring up discussions of disagreement on relative priorities? Is it any of our business what your priorities are, or should we get any "vote" in trying to sway those priorities? Or should we just infer that something not being worked on has no priority and just give up?

I mean, the classic response here of "just submit a PR" is fine, because it shifts the discussion away from "it's not my (project owner's) priority" to "if it's your priority, do something about it." But that fatally assumes that anyone with a priority to have something fixed has equal amounts of time and ability to fix it.


Speaking only for myself, I think this thread's issue is a super high priority bug, and its a deep shame it hasn't been fixed. However, I have neither the time (I'm busy teaching, writing, and building a startup) nor the ability (I am sure I could figure out Babel's code base, but I know zero of it, so having to learn it from scratch to fix a bug would probably take me way more time).

And btw, my priority judgement is based on the fact that it's a clear spec violation to not support it, and I (as a non project member) feel spec compliance/adherence issues should get the utmost priority in a project like this. Otherwise, gotchas like this (documented or not) make me trust a project less, because I'm wondering if I use a language feature if it'll actually come out on the other side as correct.

But that's just my priority judgement. I don't claim it's yours. I only bring it up to say, there should be a polite and reasonable way for us non-project members to make suggestions about priority shifts. Or, at least, I wish there was a way to do that. Maybe you feel that's out of bounds.

Shaming is a terrible way to have the debate of priorities. But it's an attempt to have that debate, where one isn't happening. I just felt maybe we should clearly identify that underlying desire and address it.

@trusktr
Copy link

trusktr commented Apr 4, 2018

I thought this was fixed? Or maybe I'm using the repl wrong. Here's the repl showing no transformation of new.target.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 4, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has PR help wanted outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Projects
None yet
Development

No branches or pull requests

10 participants