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

Avoiding duck type checking #61

Closed
Download opened this issue Mar 16, 2016 · 17 comments
Closed

Avoiding duck type checking #61

Download opened this issue Mar 16, 2016 · 17 comments

Comments

@Download
Copy link

I created this issue because I feel that the discussion in class decorator optional opts #23 has become too broad to progress further. This is intended as a much narrower problem definition. I'm hoping we can use this issue to explore solutions so we can ultimately create a very concrete proposal.

Problem

There are members in the community that want to enable their decorators to be used with and without braces alike: @deco should have the same effect as @deco(). I'll call these 'universal decorators'.
Currently, to achieve this, we need to inspect the arguments passed to the decorator and infer from those whether the function is being called as a decorator (by the runtime), or directly from client code. For this we depend on checking e.g. argument.length, typeof <argument>, etc. This creates hard to read, fragile and error-prone code.

Constraints

The discussion in #23 explores many options, but for this issue I will set up some constraints to limit options:

  • Fully backwards compatible. No code changes needed.
  • It should be possible to determine beyond doubt whether we are being called as decorator or not

Options

I'll kick off with two options, both maybe far from ideal, but satisfying above conditions:

Augment the arguments object

The runtime sets some flag on the arguments object it passes to the decorator, so inside the decorator we can check this flag:

function deco() {
  if (arguments.decoratorInvocation) {
    // called as decorator by runtime
  }
  else {
    // called as decorator factory by client code
  } 
}
Temporarily augment the decoration target

The runtime sets a non-enumerable flag on the decoration target before invoking the decorator and removes it again once the decorator has run. Inside the decorator we can simply check this flag:

function deco(target) {
  if (target && target.__es_decorator_invocation__) {
    // called as decorator by runtime
  }
  else {
    // called as decorator factory by client code
  } 
}

Feedback on these options, and of course other options, very welcome!

@silkentrance
Copy link

@Download Since the arguments semi-array is maintained by the engine I believe it rather difficult for for example babel to mimic such behavior as shown in the first example.

The second example is more like it, provided that Javascript stays single threaded and that not two competing threads share the same memory and engine related state.

@silkentrance
Copy link

moved to #62

@Download
Copy link
Author

@silkentrance I think it would have been best if decorators worked the way you describe from the very beginning. We would have no need for duck typing and it also offers room for future growth. The way decorators are set up in the current spec, if we ever find we need more information from the system available inside the decorator function, we are forced to add or change the parameters, thereby breaking BC... But alas this is already the situation we are in. As you say yourself this change would require all decorators to be rewritten.

As such I have to ask the question, given the title of this issue, whether your proposal is not overkill just for avoiding duck typing? I know your proposal offers many other benefits, but that again broadens the scope so much.

Maybe it would be more reasonable to try to look at what problems we want to solve. After Jay Phelps explained the left hand side expression nature of whatever follows the @ symbol, I more or less gave up the idea of the runtime treating @deco and @deco() (with parentheses) the same, because it suddenly clicked how powerful and flexible left hand side expression really is and how severely we would have to limit that in order to achieve that. We would have to limit the expressive power of @, and require many, many decorators to be rewritten. In my mind, that trade-of is not worth it.

So I am now opting for the second best thing, which is making writing universal decorators really simple and robust for decorator authors. Hence this new issue, which focuses purely on getting rid of the nasty duck typing that currently makes writing universal decorators so tricky. I am hoping we can achieve that in an elegant way without breaking BC.

I do like your proposal though. It would have been much better that way. But it is a radical change from the current spec. Maybe it deserves it's own issue?

@silkentrance
Copy link

@Download well I thought about making it a different issue, but since you embarked on the duck typing thing which is what @jayphelps originally sort-of complained 'bout, I thought it best to put it in here. But if you will, I can make this a new issue and remove any references to this right away.

As for the @deco() or not @deco part, your proposal is actually the same, lest the built-in object part. And since you offered this up for discussion, well...

Moved to #62.

@Download
Copy link
Author

I think it is better because we can then get more focused discussion on each option so yeah, thanks!

@Download
Copy link
Author

Thinking more about the two options I mentioned. @silkentrance is probably right that augmenting arguments is too difficult. And it occurred to me that we still want people to be able to manually perform the decoration in a natural way. Having to set a flag on the target manually wouldn't be very cool...

What about a third option?

decorator.decoratorCall

We could introduce a function decorator.decoratorCall. The runtime would check whether the decorator has such a function and call it when it's there. If it's not there, it would default to the current behavior. We could then use it as a hook to create the flag we need ourselves:

function decorator(...args) {
  function decoratorFactory(optional, args) {
    return (target, props, desc) => {
      // decorate target using optional args
      return target;
    }
  }

  return decorator.decorating
    ? decoratorFactory()(...args)
    : decoratorFactory(...args);
}
decorator.decoratorCall = function(...args) {
  decorator.decorating = true;
  const result = decorator.call(...args);
  delete decorator.decorating;
  return result;
}

Based on above code, we could design a @universalDecorator to be applied to the decorator factory that adds the boilerplate to hook into decoratorCall:

@universalDecorator
function decoratorFactory(optional = 'default', args = 'args') {
  return (target, props, desc) => {
    // decorate target using optional args
    return target;
  }
}

I actually like this option! It's:

  1. Fully backwards compatible
  2. Can get us to the 'only write decoratorFactory' ideal from class decorator optional opts #23
  3. Uses decorators to make writing universal decorators easy :)
  4. Is completely opt-in

Downsides:
2. Opt-in, so as clients we are not sure if the decorator we are using is a universal one or not

@Download
Copy link
Author

@cztomsik @Aaike @isiahmeadows @lukescott @jeffijoe @loganfsmyth @alexbyk

Don't want to waste you guys' time, but I am pretty excited about the third option described above, because it would allow us to write universal decorators like this:

@universalDecorator
function deco(optional = 'default', args = 'args') {
  return (target, props, desc) => {
    // decorate target using optional args
    return target;
  }
}

...while still being fully backwards compatible. What do you think?

@lukescott
Copy link

It pollutes the object. It's not very clean. To be honest I would rather break backwards compatibility to achieve a simple and more straight forward solution.

@silkentrance
Copy link

@Download the problem with your proposal is that it is not side-effects free. Meaning that if Javascript becomes multi-threaded, which it surely will, setting and deleting properties on a given object is prone to error, especially if those threads share the same code base, as these operations in context of the above presented examples are not atomic. As such, you will have to put in additional guards, such as semaphores and the likes.

And even with current availability of promises, and promises returning decorated classes or whatever gets decorated, there might still be side effects.

@dead-claudia
Copy link

That, and it's not easily transpiled. Syntax changes that can't just be desugared and/or transpiled are harder to get accepted for the very problem it's harder to test the use case for it.

@Download
Copy link
Author

Mmmm

I am not sure you guys get what I'm proposing. Maybe I should have clarified, but the @universalDecorator in my example is user code.

What I'm saying is that we could augment the spec to, instead of calling the decorator function itself, call a method of the function, if it exists. Then, people like me can choose to implement a @universalDecorator themselves if they like.

So if I want to intercept the decorator call, I give my decorator function a method:

function decorator() {}
decorator.decoratorCall = function(){
  // do whatever you want here. This will be called
  // by the runtime i.s.o. de decorator function itself
}

@silkentrance
Copy link

@Download in your initial example

decorator.decoratorCall = function(...args) {
  decorator.decorating = true;
  const result = decorator.call(...args);
  delete decorator.decorating;
  return result;
}

You set the decorating flag on entry and unset it on exit. This is the part I was complaining about, that it is not side effects free.

And I get that you also provided it to prevent yourself from otherwise duplicating shared code.

@Download
Copy link
Author

Yeah, but again, this would be user code. If you don't like this code, you just don't use it, but I don't mind it, so I can use it, just because I have a way to trap the decorator call.

I have been thinking about it some more, but I think we can simplify it even further... If we can have the runtime just call Function.call on the decorator. It can do this always and people like me can override Function.call to intercept the decorator call.

I made a small POC.

Basically, I think we can make this change and be backward compatible at the same time. We then leave it to userland code to determine if/how to deal with the different types of invocations. I think my POC shows that writing a universal decorator would be relatively simple. It requires no duck typing this way.

Please have a close look at my proposal.

All I'm proposing is that the runtime does not call the decorator directly, but instead invokes it's call method. This should have no effect whatsoever on existing decorators, but allows userland code to intercept that call and hence deal with calls without parentheses without having to do duck typing.

  • Fully backward compatible
  • No nasty flags, no globals, no new methods/classes etc
  • Only one thing changes: the runtime calls decorators via their .call method
  • Userland code can do the rest
  • This also still allows manual usage: deco('my', 'args')(target)

@Download
Copy link
Author

@wycats I don't want to take up your valuable time, but I think I might have something here.

I am proposing that the runtime calls decorators (e.g. the function that the expression to the right of the @ symbol evaluates to) not directly, but via it's call method. Though it would still require a bit more work to create 'universal decorators', it wouldn't depend on brittle duck typing, while at the same time opening up more ways for userland code to control the decoration process. I think it would address most of the problems voiced in #23

@silkentrance
Copy link

@Download nice but it still some extra burden on the developer. however, the duck typing @jayphelps referred to was about the parameters passed to the decorator, i.e. target, attr and descriptor and whether it is being used as a class decorator or a method/property decorator.

Nonetheless, the call approach has some nice ring to it and does not require extension to the language by introducing yet another built-in type.

@Download
Copy link
Author

@silkentrance

the duck typing @jayphelps referred to was about the parameters passed to the decorator.

Yes, I know. But he discussed this in the context of having to do duck typing in order to be able to implement universal decorators. But you are right. If we would change the method signature and pass a typed object that would open up lots of possibilities and reduce the need for duck typing in more situations. However, I think that the situation where you have to do duck typing to figure out whether the function was called as a decorator or as a decorator factory, is a common one. I was writing the code like that and thinking by myself 'I must be doing this wrong. This is much too brittle'. Then I did a Google search and ended up here :)

@silkentrance

the call approach has some nice ring to it and does not require extension to the language by introducing yet another built-in type.

Thanks. And even better, it is fully backward compatible. I also think it would be a quick and simple change to implement in e.g. Babel.

@Download
Copy link
Author

I have done a much more readable write-up of my proposal. I think it addresses all your comments above. Thanks for the feedback guys. Closing this issue and hoping for your support of my proposal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants