class decorator optional opts

[cztomsik]

Is it really necessary to make difference between regular class decorators and those accepting arguments?

Because it would be great if we didn't have to test presence of arguments, how would you implement @dec with optional arguments?

It feels inconsistent - I'd rather have to return function all the time instead of doing nasty isClass checks

@dec({...})
class SomeClass{
  ...
}

@dec
class AnotherClass{
  ...
}

[dead-claudia]

I believe this is valid syntax:

@dec()
class AnotherClass{
  // ...
}

That should solve your use case.

[cztomsik]

Yeah, I know but it feels weird.

[dead-claudia]

True. I was thinking that as I typed that reply. And as for the checking, I
don't think it's worth adding that boilerplate to the proposal, though.
Especially considering the recent disdain on es-discuss over proposed new
language features that are better off as macros.

On Sat, Jun 20, 2015, 06:55 Kamil Tomšík [email protected] wrote:

Yeah, I know but it feels weird.

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

[Aaike]

i have been struggling with the same thing. I wanted an easy way to create decorators that could be used with or without the parenthesis.
I ended up creating some utility functions to help with that, it's probably not the best way of doing it , but it works : https://github.com/gooy/js-deco

[dead-claudia]

If you want a proper, complete checker, you'd have to write a helper function for it, and it could still be theoretically fooled if it's given something that looks enough like a duck.

[dead-claudia]

Although in practice, it is not quite that likely.

[lukescott]

Is there any reason to not require the outer function to always return a decorator function as the OP suggests? It would make things far simpler.

[cztomsik]

@lukescott yeah, that was my point - but I'm afraid it's too late as there is a lot of libs already using it.

[jeffijoe]

@lukescott @cztomsik I think always wrapping in a function makes sense, to avoid confusion. Existing libs shouldn't be hard to fix?

[dead-claudia]

Shouldn't be, as far as I know.

On Wed, Nov 4, 2015, 05:42 Jeff Hansen [email protected] wrote:

@lukescott https://github.com/lukescott @cztomsik
https://github.com/cztomsik I think always wrapping in a function makes
sense, to avoid confusion. Existing libs shouldn't be hard to fix?

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

[jeffijoe]

I have 2 production app's that are heavily powered by custom decorators, so I know for a fact that it won't be difficult.

[cztomsik]

Do you really want to see warnings like these all over the internet?

[dead-claudia]

@cztomsik Either they didn't think of this, or they don't trust this:

function InjectableImpl(target, prop, desc, opts = {}) {
    // ...
}

function Injectable(opts, prop, desc) {
    // You can check `opts` and/or `prop` as well, if you want extra insurance.
    if (arguments.length === 3 && typeof desc === "object") {
        return InjectableImpl(opts, prop, desc)
    } else {
        return (target, prop, desc) => InjectableImpl(target, prop, desc, opts)
    }
}

It's possible to overload a function and just check the types to make sure it's not being used as/like a decorator. Should this be able to fix the other half of the issue?

[dead-claudia]

That's also possible in TypeScript as well. You just need to overload the function type signature.

function InjectableImpl(
    target: any,
    prop: string,
    desc: PropertyDescriptor,
    opts: Options = {}
) {
    // ...
}

interface InjectableDecorator {
    (target: any, prop: string, desc: PropertyDescriptor): any;
}

function Injectable(opts: Options): InjectableDecorator;
function Injectable(opts: any, prop: string, desc: PropertyDescriptor): any;

function Injectable(opts: any, prop: string, desc: PropertyDescriptor) {
    // You can check `opts` and/or `prop` as well, if you want extra insurance.
    if (arguments.length === 3 && typeof desc === "object") {
        return InjectableImpl(opts, prop, desc)
    } else {
        return (target: any, prop: string, desc: PropertyDescriptor) => {
            InjectableImpl(target, prop, desc, <Options> opts)
        }
    }
}

[dead-claudia]

So, it should be possible to optionally take arguments, if you're willing to accept a little bit of boilerplate.

For a general solution assuming a single options object is taken:

function makeDecorator(callback) {
    return function (opts, _, desc) {
        // You can check `opts` and/or `prop` as well, if you want extra
        // insurance.
        if (arguments.length === 3 && typeof desc === "object") {
            return callback({}).apply(this, arguments)
        } else {
            return callback(opts !== undefined ? opts : {})
        }
    }
}

// Example usage:
var Injectable = makeDecorator(function (opts) {
    return function (target, prop, desc) {
        // do stuff
    }
})

Or if you'd prefer just a single function:

function makeDecorator(callback) {
    return function (opts, prop, desc) {
        // You can check `opts` and/or `prop` as well, if you want extra insurance.
        if (arguments.length === 3 && typeof desc === "object") {
            return callback({}, opts, prop, desc)
        } else {
            if (opts === undefined) opts = {}
            return function (target, prop, desc) {
                return callback(opts, target, prop, desc)
            }
        }
    }
}

// Example usage:
var Injectable = makeDecorator(function (opts, target, prop, desc) {
    // do stuff
})

[cztomsik]

Why do we have to do this in the first place?

if (arguments.length === 3 && typeof desc === "object") {
    return InjectableImpl(opts, prop, desc)
} else {
    return (target, prop, desc) => InjectableImpl(target, prop, desc, opts)
}

It's cryptic!

[dead-claudia]

@cztomsik All I did was check the arguments and a simple sanity check to ensure the last argument at least somewhat looks like a descriptor. It's just an overloaded function, where it either applies the decorator with no options, or binds the options and returns a decorator. Nothing cryptic there, unless you're referencing the ES6 arrow function. It's just 5 lines of code.

And like I also commented, there's still a way to conceal all that behind a function. And I ended up using another similar technique to ensure both of these worked as anticipated:

var f = Param(Foo)(function (foo) {
  return foo + 1
})

class Foo {
  @Return(Types.Number)
  bar() { return 1 }
}

Believe it or not, that overload is slightly more complicated than this.

[cztomsik]

Sorry, my point was that spec itself leads to cryptic code - in its current state it's hard to explain to coworkers.

If there was no if/else, it would have been easier. What's an actual usage of paramless version - in which case it is better than regular (target, prop, desc) => ... decorator?

And if there is any - is it really worth of bugs it may cause - or extra safety code you will have to write?

[dead-claudia]

And in general, most decorator-based libraries and frameworks coming out now either always require a call like Angular's Injectable(), or always require a call unless the decorator never takes arguments, like several in core-decorators.

[cztomsik]

Yeah, and that's why they have such big alert in docs ;-)

[Download]

Sorry for chiming in here but this issue ranks high in Google on my searches for information about using braces or not with decorators...

I wrote a small test case, but the results are not what I am expecting. From reading this issue I understand this may actually be correct though:

describe('decorator', ()=>{
    function myDecorator() {
        return (target) => {target.decorated = true; return target;}
    }

    it('works when called with braces', ()=>{
        @myDecorator()
        class Test {}
        expect(Test.decorated).to.equal(true);
    })

    it('works when called without braces', ()=>{
        @myDecorator
        class Test {}
        expect(Test.decorated).to.equal(true);
    })
});

Output:

decorator
  √  works when called with braces
  ×  works when called without braces
      AssertionError: expected undefined to equal true

I would have expected the same results in both cases...
If I add logging to the myDecorator function, I see that it is called twice... So probably this is exactly what you guys are talking about in this issue?? I have to look at arguments.length somehow?

Can anyone here maybe refer me to the section in the spec that describes this?

And to add my 2 cents... I find the current behavior very unexpected so maybe other devs will feel the same... It may be too confusing for people. I think it would be best if decorators worked the same way no matter what they are decorating,,,

[dead-claudia]

@Download My workaround is admittedly a hack, but it's a conceptually simple one that doesn't really require much code.

[cztomsik]

@Download Yes, it's totally weird

[dead-claudia]

@Download Also, your example isn't correct. And it shouldn't work. myDecorator returns a function, and ignores all its arguments. So I would expect the first example to return the arrow function. Here's what I would expect your example to be, if it were to be passing:

describe('decorator', () => {
    function myDecorator() {
        return (target) => {target.decorated = true; return target;}
    }

    it('works when called with braces', () => {
        @myDecorator()
        class Test {}
        // Test is a class.
        expect(Reflect.isConstructor(Test)).to.equal(true);
        expect(Reflect.isCallable(Test)).to.equal(false);
        expect(Test.decorated).to.equal(true);
    })

    it('works when called without braces', () => {
        @myDecorator
        class Test {}
        // Test is an arrow function.
        expect(Reflect.isConstructor(Test)).to.equal(false);
        expect(Reflect.isCallable(Test)).to.equal(true);
        expect(Test).to.not.have.property("decorated");
    })
});

[silkentrance]

Close, invalid/won't fix.

[Download]

@silkentrance I hear reasonable concerns voiced here on a proposal that is still WIP...This issue ranks high in Google when searching for info related to this... So please don't close just yet... At least these concerns should be adressed in the specs somehow.

[dead-claudia]

@Download

Is my last response at all an acceptable workaround?

@wycats Could you weigh in on this?

[Download]

@isiahmeadows Your response is more or less describing how it works now, however I was describing how I would expect it to work.

Based on my experience with other languages (notably Java which has annotations with a very similar syntax) and on 'common sense', I would expect @myDecorator (without braces) to be functionally equivalent to @myDecorator() (with braces). I expect the transpiler to notice the missing braces and 'insert them' as it were. If it did that, both cases would yield an arrow function, which would in both cases be called with the class as an argument.

I am probably missing something very big, but at this moment I see no advantages to not doing it this way... only a lot of confusion... But there probably is a very good reason not to do it this way... I just don't see it.

[dead-claudia]

My thing was that I feel that workaround should be sufficient for most use
cases. I don't see the need to complicate the language further just for
this.

On Tue, Mar 1, 2016, 09:26 Stijn de Witt [email protected] wrote:

@isiahmeadows https://github.com/isiahmeadows Your response is more or
less describing how it works now, however I was describing how I would
expect it to work.

Based on my experience with other languages (notably Java which has
annotations with a very similar syntax) and on 'common sense', I would
expect @myDecorator (without braces) to be functionally equivalent to
@myDecorator() (with braces). I expect the transpiler to notice the
missing braces and 'insert them' as it were. If it did that, both cases
would yield an arrow function, which would in both cases be called with the
class as an argument.

I am probably missing something very big, but at this moment I see no
advantages to not doing it this way... only a lot of confusion... But there
probably is a very good reason not to do it this way... I just don't
see it.

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

[lukescott]

Complicate the language? Checking arguments.length is complex. Expecting @myDecorator and @myDecorator() to "do the same thing" seems reasonable to me.

Why is the decorator syntax like this?

function myDecorator() {
  if (arguments.length === 3) {
    let [target, name, descriptor] = arguments;
    return actualMyDecorator(target, name, descriptor);
  } else {
    return actualMyDecorator;
  }
}
function actualMyDecorator(target, name, descriptor) {
  // do stuff
  return descriptor;
}
@myDecorator method() {} //-> myDecorator(prototype, "method", methodDesc)
@myDecorator("foo") method() {} //-> myDecorator("foo")(prototype, "method", methodDesc)

When it could be this?

function myDecorator(target, name, descriptor, ...args) {
  // do stuff
  return descriptor;
}

@myDecorator method() {} //-> myDecorator(prototype, "method", methodDesc)
@myDecorator("foo") method() {} //-> myDecorator(prototype, "method", methodDesc, "foo")

Checking length and types isn't fool proof either. What if you expected 3 arguments? What if the first argument was an object? Seems very fragile to me.

And to add to this, what if a decorator is used on a method or a class? How do you know which one? More type checking? I added issue #40 for that. The general idea is you do something like this:

var myDecorator = Object.createDecorator({
  classMethod(target, name, descriptor, ...args) {
    // do stuff
    return descriptor;
  }
});

The above would only support myDecorator on a class method. If you wanted to also use it on a class:

// Alternative: new Decorator({...})
var myDecorator = Object.createDecorator({
  class(Class, ...args) {
    // do stuff
    return Class;
  },
  classMethod(target, name, descriptor, ...args) {
    // do stuff
    return descriptor;
  }
});

Notice how each function signature matches the context of where the decorator is being used.

Also, having something like Object.createDecorator makes a decorator more intentional. It prevents a plain JavaScript function, object, or plain value from being used as one.

[Download]

@alexbyk and @silkentrance I think your disagreement is based on one fundamental difference: alexbyk really wants decorators to work with or without parentheses, while silkentrance simply does not see any need for that.

The point is this: alexbyk is not alone. We've seen many others mention a desire to be able to create 'universal' decorators. And it has been sufficiently proven that this is a) complex to do and b) sometimes even impossible. So maybe we should focus on finding a way to make creating 'universal' decorators easy/possible? Preferably while maintaining backwards compatibility with existing code?

I wonder whether it would be possible to abuse the arguments object to be able to tell the difference?

function universalDecorator(target) {
  if (arguments.decoratorInvocation) {
    // decorate target with default options
    return target;
  }
  else {
    const someOption = target;
    return target => {
      // decorate target with someOption;
      return target;
    }
  } 
}

It would still be work to create 'universal' decorators, but at least the test would be simple and robust.

[lukescott]

You could bind something to this inside the decorator function. If this is undefined it is being used as a factory (in strict mode). And if this is defined (to the target?) then it is being called as a decorator.

decorator.call(target, prop, descriptor)
// vs
decorator('arg1', 'arg2').call(target, prop, decorator)

[dead-claudia]

Is there a reason why you couldn't just always require a closure, and implicitly convert @decorator to @decorator()? I feel it's the simplest solution to this problem, and the safest, considering Angular, Aurelia, and friends.

/cc @wycats (your opinion is worth way more than anyone else's here)

---

Yes, I know I've come a full 180 on that idea, if you've been paying any sort of attention. I'm staring very intently at several libraries/frameworks, including those I mentioned, which make significant changes dangerous. Also, this proposal was pretty stable until a couple months ago, when this bug blew up.

[lukescott]

@isiahmeadows Take a look at Jay Phelps example:

All that said, my point is that there is not two different ways of applying a decorator there are many ways! Here are a couple I believe are all valid:

class Examples {
 @function () { alert('wow'); }
 first() {}

 @(false, function () { alert('wow'); })
 second() {}

 @(() => () => () => alert('wow'))()()
 third() {}
}

// This might even blow your mind
(function () {
 class Foo {
   @this
   first() {}
 }
}).call(function () { alert('wow!!!!'); });

The simple answer is you can't. With @decorator(), decorator isn't a decorator - the result of it is. The only way to do it is with more restrictive grammar, where @decorator() is no longer a function call.

Angular and fiends can make both work with the existing grammar, if the decorator function is simplified to the point where it is easy to know whether it is being called as a factory or decorator. Binding this for the decorator call seems like the simplest solution to me.

[silkentrance]

@alexbyk never mind. the code works as expected. however, and I agree with @Download that our differences are in the way decorators are realized and used and not their implementation on the user side.

@Download the problem with passing in a this object and thus "binding" the decorator to that object is IMO not feasible as one could definitely use bound functions as decorators. These would then be bound to some external object. And overriding that binding would then result in a dysfunctional decorator.

E.g.

class IngeniousDecorationFramework
{
     theDecorator(target, attr, descriptor)
     {
         this.log('now decorating ' + target.name);
         ...
     }
}

const instance = new IngeniousDecorationFramework();

const theDecorator = instance.theDecorator.bind(instance);


@theDecorator
class DecoratedClassUsingABoundFunction
{}

Not that I think that this would a wise thing to do, but it is possible.

[Download]

@silkentrance What about the use of arguments, as in arguments.decoratorInvocation, which would be a boolean set to true by the runtime for decorator invocations? See also my comment a few posts back.

[dead-claudia]

Are you all aware this could have big implications for a massive number of
users (most notably Angular 2, currently in beta, but several others as
well)? It's too late to make large, sweeping changes, since many people
keeping near the edge have already started the process of upgrading to
Angular 2 in production.

I'd rather not see this bug closed simply from ignoring that fact.

On Sat, Mar 12, 2016, 17:06 Carsten Klein [email protected] wrote:

@alexbyk https://github.com/alexbyk well I could continue referring to
Java or C# annotations, where we find a similar syntax.

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

[Download]

@isiahmeadows Yes I'm well aware. As they should be aware that this is a relatively early proposal, which may be subject to change. But if breaking changes can be avoided while still improving the situation, that would be a worthy pursuit no?

So... might it be possible to augment the spec in such a way that the scenario discussed in this issue, 'universal decorators' so to speak, would be easy to achieve without fragile duck-typing? I think it would satisfy most of the participants here if that were the case.

Hence my proposal to use some kind of non-obtrusive flag to indicate to the function being called whether it is being called 'manually', or by the runtime as part of the decoration process. It might be possible to place such a flag on the arguments object (without actually adding an extra argument to the call, which might break said duck-typing). Or maybe there better ways to signal a decorator invocation?

All we really need is a reliable way to do:

if (/* some flag indicates we are called as a decorator by the runtime */) {
  // do my decorator thing
} else {
  // do the manual invocation thing
  // then return the actual decorator to be called by the runtime
}

[lukescott]

@isiahmeadows Angular 2 users are using decorators, not making them, correct? So what we are talking about affects the authors of Angular 2, not the users.

@silkentrance Bound functions are a good point. Although there are already cases in the language where passing a bound function would yield broken behavior. Anyway, I believe @new decorator() is valid syntax anyway, so it's probably not a good idea.

@Download I doubt anything could be added to arguments. A number of properties on arguments have been deprecated. And new.target was added instead of augmenting arguments. I'm not even sure if that could be emulated in babel. IMO it doesn't feel right to me either.

Jay's suggestion so far is best:

we could instanceof check for instead of relying on brittle duck typing.

function decorator(...args) {
  if (args[0] instanceof D) { // D?
    let {target, property, descriptor} = args[0];
    // decorator
  } else {
    // factory
  }
}

[Download]

@lukescott
I see two problems with the instanceof check:

1. Which argument to check? We have target, prop and desc, but only target is there in case we are decorating a class.... This would be simple if we could just wrap up those parameters in a special DecoratorAttributes object or something, but that would break all existing code. We shouldn't mess with the arguments like that if we want to maintain backward compatibility
2. instanceof can be problematic when instances cross frame boundaries, web workers etc.

I can imagine augmenting arguments is hard though... So are there other candidates that make sense? Ideally we want some place only visible inside the decorator that we can safely manipulate before calling the decorator and restore to it's original state afterwards.... Maybe create a flag on the target object and delete it afterwards? (I know, super ugly, but would actually be very straightforward to implement and if you name the flag something unique, say, __es_decorator_invocation__, would be very safe in terms of backwards compatibility). I hope we can come up with something more elegant than that, but even such an ugly flag would beat the brittle duck typing we have now imho.

[lukescott]

@Download You would have to wrap the special arguments into a special object. Also, I highly doubt that web workers would be a problem as decorators are used on initialization. It's more than likely the decorator is being imported.

Backwards compatibility isn't much of an issue for the following reasons:

1. The decorator code would change, not the use of the decorator. Angular 2 authors would update their code. Users of Angular 2 would not have to update their code. At worst they would have to choose the correct compiler.

2. There is no browser support. Support is being achieved by compilation (babel, etc.). Versioning takes care of older versions. Angular 2 uses TypeScript.

3. This proposal is in Stage 1. Babel disabled decorator support in version 6 (didn't it?)

[Download]

@lukescott I think that backward compatibility is not such a small problem. Angular devs will survive and adapt, but there will be a lot of code lingering around for a long time that simply does not get updated. But much more importantly, it may end up being very off-putting for early adopters and people interested in using decorators... they may take a wait-and-see position, not willing to risk having to rewrite their code yet again.

I would hate for this proposal to lose momentum. I think the Babel folk temporarily removing it has already done a lot of damage. Let's not make that worse. So I think we should only consider breaking backward compatibility if it will bring a very clear benefit that cannot be gotten without breaking back. compat.

The way I see it, the clear benefit I am after is being able to write 'universal' decorators without brittle duck typing. For that purpose, I only require some clear sign. This could be manny different things as long as it's clear and robust. Changing the function arguments may bring other benefits, but the cost of breaking back. compat outweighs those imho. But we can get this one huge benefit without breaking backward compat so that is what I am now voting for because I don't think we'll ever get more out of this issue and frankly, I don't need more.

[lukescott]

@Download This is stage 1:

Stage 0 - Strawman - Expected Changes: N/A
Stage 1 - Proposal - Expected Changes: Major
Stage 2 - Draft - Expected Changes: Incremental
Stage 3 - Candidate - Expected Changes: Limited / Only deemed critical
Stage 4 - Finished - Expected Changes: None

(From https://tc39.github.io/process-document/)

People jumped on this at stage 0. We wouldn't even be having a discussion about backwards compatibility if it weren't for compilers like babel. At the stage 1 major changes should be expected. Those who use features at this stage should expect to rewrite their code.

I wouldn't consider any feature in-process to be "stable" until stage 3. Somewhat stable at stage 2 if you really want to be generous.

With the changes themselves, wrapping up the arguments into a single object doesn't change much for the decorators themselves. They can still get to the target, property, and descriptor. In many cases this is a simple rename. For detecting @decorator vs @decorator(...) it's less boilerplate.

This really isn't something we need to be agonizing over at this stage.

[Download]

@lukescott I understand the definitions of the stages, but I don't believe in them.

People are as people are. The way I see it, this proposal received a very enthusiastic welcome because people really want decorators. Bad. So bad in fact that they are willing to base significant parts of the new major version of their very popular and influential framework on it (Angular 2). They jumped on this at stage 0 for a reason. They want this and it being an 'official' proposal for the language was enough for them to embrace it.

But then things changed and Babel removed support (temporarily...). Suddenly, having these cool decorators ended up as a problem. Of course you know it can happen with stage 0/1 proposals, but that does not mean you want it to happen. Mostly, you just want this thing to move through the proposal phase as quickly as possible and end up implemented in the major browsers so you can use it and be all vanilla JS!

Breaking backwards compatibility now may end up killing that early enthusiasm. I for one don't think universal decorators warrant that, especially because I believe we can have the cake and eat it too.

I think that whether BC may be broken or not is a significant constraint so I created a separate issue that strictly disallows it. We can then continue using this issue to explore avenues without considering BC.

[dead-claudia]

@lukescott If a given API is changed to use different idioms, even if they're similar, it's still breaking to both the API creators (Angular 2 devs) and API consumers (Angular 2 users). Just thought I would clarify.

@Download I'm not too worried Babel dropping official support is going to break too many hearts, considering this plugin was made to fix that very problem.

As for my opinions on what should/shouldn't be done, I think they're pretty well known at this point. I think I'm going to sit back for now, instead of letting my overly opinionated self get out of control. 😄

[lukescott]

@Download The purpose of stage 1 is to demo the feature as a "high level api" and to "identify potential challenges". Whether you agree with the process or not, it is set up that way for a reason.

I believe that the Angular 2 guys are smart enough to know that the decorator functions could change. The syntax itself, which is used by their users, isn't changing.

Much larger things happened with the development of the es6 spec. For example, when class methods changed from enumerable to non-enumerable. Changes are inevitable, especially with proposals.

[silkentrance]

@lukescott I like the idea of introducing a new built-in object. Wouldn't it be best to discuss this over here #62?

[silkentrance]

Since the discussion here revolves around whether #62 and the proposal in there should be made part of the ECMAScript standard, and also since this discussion may not lead to any conclusion, perhaps we all move to #62 instead.

There, @Download, who took the initiative in #61, and I, who took the liberty of producing a proposal from the assorted information found in here and there, lest the bashing and other unrelated stuff, we might come to a conclusion that will hopefully benefit all.

And sorry for being somewhat obnoxious, but this needed to be stirred in order to make the discussion productive again.

And even @cztomsik might find his original case being taken care of in the presented proposal.

[le0nik]

Encountered the same problem in my projects. Created this helper function: https://github.com/le0nik/decorate-with-options.

[dead-claudia]

BTW, the current state of the proposal can be seen here.

[Download]

To be honest I am a bit disappointed. There has been lots of discussion here on this and other related issues about the duck typing / decorator vs factory / with/without parentheses issues, but I see nothing mentioned on it anywhere in the new proposal. It looks like the whole discussion has been completely ignored...

(could be I'm reading it all wrong and it has in fact been addressed or at least mentioned somewhere... if so, please point me to the relevant text because I can't find it)

[silkentrance]

@Download the overall discussion was moved. And there is also a new issue there that, unfortunately, tries to revive this one... #75 (comment)