proposal-decorators-previous

proposal-decorators-previous copied to clipboard

Consider:

@foo
class A {
}

@foo(A)
class B {
}

I am attempting to provide a decorator that accepts fully optional arguments including optional class parameters. Is there some idea brewing how to reliably determine the context of such things?

Given the current spec as I understand it (using babel-plugin-transform-decorators-legacy), there is no way to distinguish the above forms. To support case 1 you would do:

  function foo (C) {
      // decorate class C
  }

To support case 2, you would do:

  function foo (p) {
      return C => {
          // decorate C using p
      };
  }

But since the parameter is a class there is no way to tell which form is being invoked.

If I could wish :) I would rather write decorators like so:

  function foo (args, C) {
      // decorate C using optional additional args
  }

The idea there is that args is whatever (if anything) is supplied in parens after the decorator name. So to handle both cases:

  function foo (args, C) {
      if (args && args.length) {
          // args is an array if @foo() of some form was used
         let p = args[0];
      } else {
          // @foo w/no args was used...
      }
  }

One could debate whether args should be an empty array to distinguish @foo from @foo() I suppose.

You can try for the 'options object' option:

function foo(opts, ...args) {
  if (args.length  0) // function was not used as a class decorator
    {cls} = opts;
    return C => {...};
  else // used as a class decorator
    return dec_cls(opts, ..args);
}

@foo
class A {}

@foo({cls: A})
class B {}

... or something like that

Thanks for the suggestion @MMeent - that is a good workaround if this ambiguity is not resolved before the spec is finalized.

I do hope we can change the argument passing along the lines I suggested there... because the current functional zen (to me) is a really an unfortunate way to express the goal here :)

I find it amazing that we had this issue on the old repo: class decorator optional opts #wycats/23, with 156 comments (!!) and, afaik, no formal response from the spec leads (or any sign that any discussion had taken place to address this concern) and I've been following the progression of the spec ever since.

And now, one year and nine months later, I see everything has been moved to a new repo. The old issues are gone, like they never existed and here, on this new repo, we get the exact same question.... Hell even the issue number is only one off: 24 vs 23. Seems that's about how long it takes for people to catch up to this hole in the spec. Where are all the ideas mentioned in the discussions on the other repo addressed? Was any consideration given to the problems mentioned? I even came up with an idea to address it myself, but never had any response from anyone involved in the spec.

This is what I wrote:

Download - Stijn de Witt - commented on 20 Aug 2016 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)

Is the issue tracker here just for fun and to show off how open source Ecma is, or will you guys actually respond to community feedback / questions?

What is the idea for this issue? Will we attempt to address it? You can just wait for confusion around this... It would be a shame if the spec was progressing and this issue that has been brought up so early and often was never dealt with.

Could someone provide a concise summary of the issue? Does the current draft, at proposal-unified-class-features, still not address it?

Really concise summary:

@decorator
class SomeClass {}

!==

@decorator()
class SomeClass

People are not happy about this.

Please just spend some time reading the feedback. People placed 150 comments in 2 years time. I can't summarize all that.

Why wouldn't overloading based on "does this look like a class descriptor" work here?

@littledan that wouldn't work for the OP which passes a class as a decorator parameter

@dongryphon I don't see the contradiction; a class looks completely different than a class descriptor, as this proposal uses. You can check typeof, for one--one is an object, the other a function.

@littledan Do class-level decorators get a descriptor? From the spec it seems like they are

function dec(cls: Class<T>, parent: ?Class<T>, elements: Array<ElementDescriptor>): { 
  constructor?: Class<U>, 
  finisher?: Finisher,
  elements?: <I forget>
};

@dongryphon Either way, class-level decorators don't just take a single argument anymore, so that should help disambiguation. The behavior in babel-plugin-transform-decorators-legacy does not match the current spec.

@littledan The root cause here seems to be user confusion about the semantics and proper usage of DeclaratorMemberExpression. The feature design seems to be contributing to this confusion.

As specified in the proposal a DeclaratorMemberExpression is expected to evaluate to a "decorator function" ie, a function whose signature is rigidly defined by the specification. This means that when a DeclaratorMemberExpression is a DeclaratorCallExpression, the function that is being called with the provided Arguments isn't actually a "decorator function" (DF) but instead a higher order function that acts as a factory for DFs. Let's call such higher order functions a "Decorator Factory Function" (DFF). It's important to know that DFs and DFFs have completely different signatures and are called at different times in the class definition process.

However, the simplistic conceptual model of decorators that is likely to be taught (and which is implicit in some of the example in the specification) completely blur the important distinction between DFs and DFF. Consider the deprecate decorator from the specificaton:

import { deprecate } from 'core-decorators';

class Person {
  @deprecate
  facepalm() {}

  @deprecate('We stopped facepalming')
  facepalmHard() {}

  @deprecate('We stopped facepalming', { url: 'http://knowyourmeme.com/memes/facepalm' })
  facepalmHarder() {}
}

Is deprecate a DF or a DFF? It turns out that the usage decorating facepalm requires that it be a DF. However the usages decorating facepalmHard and facepalmHarder require that it be a DFF. That means a some points in processing class Person deprecate will be invoked as a DFF and at other points as a DF. How does it determine which kind of invocation it is responding to? How is deprecate actually coded to make this work?

export function deprecate(...DFFargs) {
  function deprecate_DF(elementDescriptor) {
     // body of the DF that is called to actually decorate elements.
     // note that it has access to the DFFargs that were passed to the DFF that created it
  }

  if (args.length == 0 or args.length > 1) return deprecate_DF;

  //if args.length is one then we don't know whether we are being invoked as a DF or a DFF
  //so we need to infer it by duck typing the argument.
  const arg = DFFargs[0];
  if (arg.kind && arg.property && "isStatic" in arg && arg.key && arg.descriptor) then {
     //looks highly likely that we are being invoked as a DF (but in rare cases we could be wrong)
     return deprecate_DF(arg);
  }
  //probably was called as a DFF with one argument.
  return deprecate_DF;
}

(Note that the duck typing would be easier and more reliable if the specification used a symbol to brand element descriptors.)

Any decorator that is going to access optional arguments is going to have to be written something like the above. This seems like a significant burden upon framework developers (or anyone who wants to define decorators). Also, this sort of confusion between the type of a higher order factory function and type of the functions that such a factory produces seems like a significant bad design smell.

It would actually be easier and conceptually cleaner if Decorator : @ DecoratorMemberExpresson always implicitly called the value of its DecoratorMemberExpression. In that cases "decorators" would exclusively be DFFs and the duck typing would not be necessary. Having to always code a decorator as a higher order function would still be a conceptual burden but it would be a smaller burden than have to code it as a conditionally higher order function.

@loganfsmyth It sounds like you're looking at an outdated version of the spec. In the current draft, there's a single ClassDescriptor given as an argument to the class decorator. Would this permit the overloading that the OP is requesting?

@loganfsmyth @littledan @allenwb Thanks for all the clarification. Now that the spec has evolved to passing an element descriptor vs the old Babel transpiler I can see a way through the mist. I still wish for the removal of the higher-order function approach (the "DFF" thing) and the ancillary type sniffing but it can be made to work. Appreciate the details!

@littledan Ah right, sorry there are so many different specs now that I've kind of lost track of which one is supposed to be the source of truth at this point.

@dongryphon I've seen type sniffing in existing decorators designed to serve double-duty as directly called functions, so it seems like this is already an OK practice in at least some of the community, for better or worse. If you have other use cases that can't be served this way, I'd love to hear about them.

@loganfsmyth Sorry about that; I put it on my todo list to clean up the repository situation here.

@dongryphon I've seen type sniffing in existing decorators designed to serve double-duty as directly called functions, so it seems like this is already an OK practice in at least some of the community, for better or worse. If you have other use cases that can't be served this way, I'd love to hear about them.

Wait, that is not a good justification for a troublesome design. You see it already because, given the current design, it is the only valid way to code a decorator that accepts optional arguments. That doesn't mean it is "ok in practice". It just means that some people have found the way to work around the design issue. Have you look for places it was miscoded or talked to people using that pattern to see how long it took them realize it was necessary? How many decorators are there out there that are just waiting to fail because somebody puts a () after them?

As was pointed out up thread (https://github.com/tc39/proposal-decorators/issues/24#issuecomment-281804327) there is a two year old 150+ comment thread (https://github.com/wycats/javascript-decorators/issues/23) about this very issue. Why are the concerns expressed there and in this thread being blown-off in this manner.

The purpose of Stage 2 is at least as much about finding and fixing problems in a proposal as it is about pushing the proposal forward to Stage 3. Let's fix this problem.

See issue #43 for a possible way to eliminate this problem.

A bit disappointing to basically see the whole discussion we had two years ago be repeated now again.... But at least it seems this time the people working on the spec are actually paying attention.

Wait, that is not a good justification for a troublesome design. You see it already because, given the current design, it is the only valid way to code a decorator that accepts optional arguments.

So true! That's the whole reason the original discussion was created. Over 150 comments were added by community members, with all kinds of different solutions to this problem. But almost everyone involved agreed that it sucked that

@deco 
class MyClass {}

and

@deco()
class MyClass {}

Don't do the same thing. Now some people found ways to work around it by duck typing the incoming arguments... But in some situations this could not be done in a fool-proof way. I guess with the new proposal you can duck type this without having edge cases that are impossible to detect? E.g. in the old design if your optional param was a function you were in trouble because you could not detect whether the function was the class itself or the optional param... Is this solved in the new spec?

But still, with duck typing, the problem remains that authors of decorators need to opt-in to making the braces optional. If they don't do the duck typing trickery, then both code samples shown above might still end up doing completely different things... So users of decorators need to check for each and every decorator whether this decorator requires braces or cannot have them or whether it does duck typing so they can be used or not at will. Lots of confusion around a very basic use case.

I'm sorry, I'm not trying to blow off any concerns, but I won't have time to read the entirety that thread right now. I thought it'd be better for me to engage here than continue to ignore the issue (which I was not aware of until just now). I don't think any open source project contributors can read every single thing everyone says about it. Having 150 comments actually makes things harder, not easier, to get up to date on.

E.g. in the old design if your optional param was a function you were in trouble because you could not detect whether the function was the class itself or the optional param... Is this solved in the new spec?

This particular thing has changed. Rather than decorators being passed the whole class, they are passed a descriptor, which is an object and not callable.

But still, with duck typing, the problem remains that authors of decorators need to opt-in to making the braces optional. If they don't do the duck typing trickery, then both code samples shown above might still end up doing completely different things... So users of decorators need to check for each and every decorator whether this decorator requires braces or cannot have them or whether it does duck typing so they can be used or not at will. Lots of confusion around a very basic use case.

This is very true. If the semantics that everyone wants are that it always acts like this, and calls the argument, that's a really easy, small change. I don't really have a problem with it. It's pretty different from allowing overloading between functions passed as arguments and this thing being invoked as a decorator, though (which, as I explained above, changed since the thread happened).

I'm more skeptical of the class-based approach described above, for the reasons described in https://github.com/tc39/proposal-decorators/issues/43#issuecomment-337710197 . I think we could do just the () part without the part of treating it entirely as classes. What would you think of that approach, @Download ?

I think I revitalised this issue with a comment that I subsequently deleted, once I realised that the spec had advanced since my last read - and because I naively assumed that the recently released Babel transform babel-plugin-transform-decorators exposed the latest Stage 2 approach - which it does not. It is effectively a bugfix and compatibility upgrade to the old legacy transform, but without the legacy suffix - slightly surprising.

That comment was motivated by the frustration of attempting to use the legacy decorator approach, again, and being unhappy that I could not consistently enforce @decorator == @decorator(), nor even detect accidental misuse, and throw - the variety of failure states, and subtleties of them are unnerving.

The approach in the latest spec certainly improves the situation, previously, one impossible edge case occurred when passing an optional constructor function, to, say, denote a type as a parameter - since there was no way to disambiguate between that, and an invocation of the decorator on a target class.

@Validate
class SomeType {

   @Validate(String)
   method(param) {}

}

Being able to duck type by inspecting the shape of the passed descriptor is better, but it still seems like there is a terrible code smell to having two possible interpretations of the same function - that adding or omitting braces changes semantics. That a decorator function can be either (or both), a factory, or (and) an explicit function, seems inherently dangerous - particularly since the onus is on the developer exposing decorators, to decide, and determine which use cases are valid. This will almost certainly lead to a wide mixture of libraries, with different approaches:

• some will insist that the developers RTFM, and explicitly always use, or omit braces / params
• some will try to unify both approaches by property detection
  • some will include bugs in that detection, for instance: if they encounter a parameter object with a kind property
• end users of such libraries will be put into a position of using a mixture of approaches
  • end users will have to commit to memory which decorators accept optional braces / params, and which do not
  • for decorators that do not, users will be required to know which require params, and which do not

It feels like a recipe for a lot of subtle bugs.

@jka6502 Thanks for writing out these thoughts. After all this discussion, which alternative are you most in favor i?

@littledan - No problem, I actually like a few of the suggestions, though the most elegant, imho, would be adapting https://github.com/wycats/javascript-decorators/issues/23#issuecomment-190834352 to the new spec, something along the lines of:

function decorate(descriptor, ...args) {}

Where @decorate is exactly equivalent to @decorate(), in that they both invoke the decorate function on the following type, method (or property - once class properties stabilise, too) directly, with a zero length args spread, i.e. just the descriptor.

Alternatively, forcing a factory for every decorator seems viable too - its the mixture of both, and inability to determine which is occurring, that makes it problematic:

function decorate(...args) {
   return (descriptor) => { /* ... */ };
}

I'd imagine some would prefer to stabilise on decorators always representing a factory function, to provide simpler memoization based on arguments - though that could be achieved either way (I actually don't mind either, as long as the parameters of a decorator become consistent, regardless of its method of invocation - i.e., the author knows whether they are writing a factory, or a function):

function decorate(descriptor, ...args) {
   return someFactory.get(...args)(descriptor);
}

It seems like a simple grammar change can accommodate the optional nature of the arguments / braces (https://github.com/tc39/proposal-decorators/issues/43 - @allenwb opened that issue with a simple and clear explanation of the changes needed, and their precedent), too.

An interesting note, that I forgot from my previous comment, and that was raised in the previous issue thread, is that there are precedents for both approaches (@decorate == @decorate() in Java Annotations, and @decorate != @decorate() in Python decorators), and there will certainly be a significant set of developers that use whatever approach is finally chosen, with presumptions about how it works.

Whether you consider that "their own fault" or not, almost analogous existing mechanisms in other languages are going to influence the likelihood of people using decorators correctly (and thus the perception of the usefulness / difficulty of decorators), and, interestingly, those with Python decorators based preconceptions will provide consistent brace / arg usage, which will not fail with equivalent semantics (==), but those with Java Annotations based preconceptions will likely mix and match, which will cause regular bugs with current ambiguous semantics (!=) unless the decorator authors attempt to unify both styles, under the hood.

All of the potential solutions suggested affect the signature of decorator functions (or boiler plate class, in one suggestion), meaning that the requirement of deeper understanding lies with those creating decorators, rather than those using them - which sits very well with me (as someone who has encountered this, predominantly, while trying to write decorators).

I think we could do just the () part without the part of treating it entirely as classes.

Couldn't agree more! If we can get @deco to be equivalent to @deco() I think that solves most peoples complaints.

As in @jka6502's comment, I would prefer:

function decorate(descriptor, arg1, arg2, arg3) {}
// And I could use the spread operator if I need.
function decorate(descriptor, ...myArgs) {}

I am not sure the other option is a good alternative:

function decorate(...args) {
   return (descriptor) => { /* ... */ };
}

Correct me if I am wrong but this would create an instance of the function each time the decorator is called which could affect the performance as I am expecting decorators to be used heavily in UIs.

@borela - On the second approach, perhaps it would be a tad slower, though the enclosed function is still precompiled, rather than entirely unique per invocation of the enclosing function, closures are very well optimised in every engine, since they are a common idiom - they are really just functions with additional 'arg' references defined in the enclosing call site.

I personally still prefer the spreadable args approach as well - just pointing out that anything that removes the ambiguity works for me (and the factory function approach clearly has some support - but is achievable manually, regardless of the approach) - I fully intend to use decorators for UI purposes too, but my, admittedly, naive testing has shown that the module load time influence of decoration is pretty negligible, even with heavy use (I've been using https://github.com/jkrems/babel-plugin-transform-decorators-stage-2-initial - though it appears to have died, and is incomplete, unfortunately).

IMHO: I don't think it is absolutely necessary to actually "improve" the situation, here. I don't find the arguments so convincing. After all the problems that are presented are the very same problems every JS developer is facing every day; they are used to these problems and trying to "solve" this problem for decorators does not really improve the situation a lot. Consider that (non-trivial) decorators will not be used by total newbies (they have this slightly "enterprisey" touch that you won't be seeing in the majority of github code bases): If you compare the situation that we have in the "regular" JavaScript world, it's not that bad actually. Consider APIs for geometric points, e.g.: some will have have a "function x()" as accessor to the x "property" like this:

p.x()

and some will have properties or fields:

p.x

Unless you RTFM you will run into problems if you use it the wrong way at run-time. And in the case of the first type of API the problem will be extremely hard to debug because doing p.x * 5 in the function case will not trigger an immediate error but will cause all kind of issues later on somewhere else in the code when the NaN will bite you.

The same holds true for omitting a required argument or passing too many arguments to most APIs out there - many of them will not make your logic fail immediately but all kind of side-effects will crash the application or make it behave unexpectedly somewhere later on. These are the kind of problems devs have to deal with every day in the JS world. And they can only avoid those problems if they know the API and/or have read the docs. So why should this spec jump through hoops in order to improve the situation specifically for decorators, only, sacrificing flexibility and possibly performance at the same time?

If API vendors/authors can come up with a consistent API and carefully implement their decorators and properly write their documentation, devs will be able to work with them in no time.

Don't get me wrong: I think simplification and removing possible pit falls is a good thing. But not for any price. So keep in mind that many devs are OK with APIs that could be "ambiguous" - that's the situation they are facing today, anyway most of the time: if the APIs are properly documented and implemented, they will be fine with that. APIs which aren't consisted or properly documented or a pain to use will die anyway soon, one would hope. Devs won't be happy, however, if they need to sacrifice performance are even be forced to like always have to append () or always need to implement a decorator as a class. That's not what they would expect from a language that's as flexible as JS, I think. At least that's not what I would expect the spec to look like.

@jka6502 We're no longer actually passing a target in. Instead, there's a descriptor passed in, and another descriptor passed out.

It sounds like the biggest thing that this thread is asking for is to make @x be the same as @x(). This should be pretty easy to do in the specification; if we make a change here, my plan would be to make this narrow change, but keep everything else the same. How would that be?

@littledan that's perfect for me, the rest of the specification seams fine as I only had trouble with optional arguments and detecting whether the class passed in was the target or just an argument.

For the record: I am pretty sure that not everyone agrees that @x be the same as @x() if you look at it from a higher level. In fact I am very positive that for the vast majority of APIs everyone agrees that someObject.someName is something different and should not magically behave the same way as someObject.someName(). Why should decorators be treated differently, here? The current solution does work. JavaScript works. Devs are used to these "subtleties", invocation makes a difference and is something specific to the API that I am using. Why change it just for decorators and force one way of doing it, if both ways work?

Why should decorators be treated differently, here?

They already are treated differently here in that foo is invoked when used as:

    @foo
    class A {}

This invocation is part of the syntactic sugar. So I don't see the analogy as applicable here. The suggestions so far boil down to making the difference between @foo and @foo() easily determined vs horrible/impossible-to-be-certain (as per current spec).