-
Notifications
You must be signed in to change notification settings - Fork 127
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
Force decorators to be higher-order functions even without parentheses #52
Comments
Its currently worked, if you are writing decorator this way: let decorator = function(...args){
let decoratorArgs = [];
let decorateHandler = function(target){
console.log(target.name , decoratorArgs)
}
if (typeof args[0] === 'function'){
decorateHandler(...args)
}
decoratorArgs = args;
return decorateHandler;
}
@decorator
class C{
}
@decorator()
class C2{
}
@decorator('foo')
class C3{
} it outputs:
So you just need to check, if first parameter is your class - if so, decorator was called without brackets and you can apply your logic in |
@PinkaminaDianePie Try you decorator with this case @decorator(function(){})
class C3{} So there's no way to distinguish this case when |
@alexbyk how about @decorator(function(){})
class C3 extends YourBaseFrameworkClass{} and check it in decorator with isPrototypeOf? |
@PinkaminaDianePie I'm proposing to solve problems with decorators, not to invent new ones. For example, I use this variant with decorators because it's simple and clear. If it make my code more complex (like requirement to extend some other class), I'll better avoid it. But thanks for your feedback |
Have a look there: https://github.com/mastilver/decorator-router/blob/master/index.js#L35 Really similar to @PinkaminaDianePie approach |
@mastilver Could you please provide an example I can copy-paste-run like @PinkaminaDianePie did? Because I described a glitch in the current design of class decorators (and provided an edge case when it's impossible to distinguish a single argument-function from the wrapped class), which can't be resolved. As far as I can see, you gave an example that isn't relevant to this topic because isn't a class decorator at all. (I made 2 words bold to show the difference) I'm asking for an example (copy-paste-run, which should solve this edge-case too) for a simple reason: if I'm right, nobody can provide such example, and that's my point - it's impossible. And, obviously, if someone can provide a solution, I'm wrong |
Sorry, my bad. |
I feel like the whole phasing of this is confusing. Decorators are never higher-order functions. If you're doing If you want to combine those two into a function that can do both, it seems like you're just asking for trouble to begin with. If something has two different semantics, pretending it can be used the same way is just asking for trouble. |
@loganfsmyth I don't try to combine anything. I'm assuming many people will try to do that. I know how decorators differ from factories, you know that too...That's a theory And here is how things work in practice:
So my point isn't "it's impossible to make universal version of decorator+factory", it sound more like "Everybody thinks it's possible... and probably tries to make it universal... and gets a surprise because it isn't". So my proposal was very simple: fix decorators syntax to avoid such problems in the future. Why? Because I expect many problems with the current syntax in the future and many "Cooking decorators: Pitfalls" blog posts. I faced this problem 1 or 2 times by myself. |
Ok. Here is a changed version of desugaring. @F("color")
@G
class Foo {
} var Foo = (function () {
function Foo() {
}
Foo = F("color")(Foo = G()(Foo) || Foo) || Foo;
return Foo;
})(); With this nobody will try to make a buggy universal version and problem won't happen |
Wouldn't the proper way to handle this then be to export the factory under a new file or property, e.g. My primary concern is that this is a total departure from how js generally behaves. Nowhere else in js does a property or identifier access magically become a call. |
@loganfsmyth About your concerns: right now neither You're right about the proper way. But problem is other people will not do it the right way, because it's a trap. This topic is a proof: look at the top, other 2 participans (I have no doubt they are smart and experienced developers) tried to convince me that there is a solution. And I'm sure there are many other people, who thinks the same way (I felt into that trap once by myself). |
To clarify my "how js generally behaves" point, the issue with this is that it means
and
would behave differently. There is no other case in JS that comes to mind where you can move an expression into an assignment, and then use the variable in its place, and have it behave differently. I get that you could certainly define it this way, but then you are trading potential confusion for decorator developers, with potential confusion for decorator users, and users are the far more common case. |
@loganfsmyth Your explanation, surely, makes sense too. But I think it's all syntax/desugaring/definition. My proposal makes If we don't define |
I have a suggestion. We have already seen enough pros and cons. How about a poll by 👍 👎 ? |
The same problem about "properties" decorator:
It's very easy to forget parentheses and there are cases when it's impossible to distinguish syntax and make a smart decorator. I also found a proof that problem exists in Angular2 docs , commit is here
Typescript inherited this problem. |
@alexbyk I think that you are missing the point here. Decorators are by default functions, yes, but calling them is left to the framework, in that case the language. In other cases you have decorators that take additional parameters, which the language cannot address for. Here, you have to call the decorator (factory) first with the set of parameters that customize the outcome of the decorated method or whatever you are decorating it with, e.g.
Why would you want to deprive both the developers and users of decorators from such a nice feature? |
Please close as won't fix. |
@silkentrance For the record, I don't disagree with your logic here or in the other replies you've just posted in this repo. That said, the tone of your comments like this are massively unnecessarily dismissive. These issues exist to foster discussion, you are in no position to demand that a given discussion be closed and finished, no matter how right you feel your opinion to be. |
@loganfsmyth it is not dismissive. I am trying to get things going again. I have reviewed every issue I commented and every issue I found so far is either outdated, or based on a personal learning experience or just invalid according to the spec. |
@silkentrance You used You can try to argue that it's possible to detect, when decorator are called as a factory, and when are called directrly by examining arguments. In that case pls read first messages above, I proved that it's not possible to make such detections and every "universal" decorator has a potential bug (a heisenbug) I used an example from the angular2 docs ( I'm repeating myself, but: The simple thing can help us to avoid all of this: Consider
|
@silkentrance Posting My complaint is that demanding that this be closed as wontfix achieves nothing but shut down discussion and makes it appear that your opinion should somehow be the final one. No-one, you or me, is in a position to decide what does and does not get closed, beyond @wycats and TC39, and it is presumptuous of you to do so. |
@loganfsmyth that is why i first answered to the issue in an rather elongated reply and then addressed the person to see whether it is fit to close this for being a won't fix issue. It seems that the person is still rather pertinent in its assumption that 50% of the developers out there will get decorators and 50%, including that person, won't. |
I've rewritten the initial message to make my proposal more formal and clear |
please consider the following
Here, the decorator returns the actual implementation of the decorator based on the parameters passed to it
The runtime will then use the decoratorImpl() returned by decorator(...) to decorate the method What is so complicated about figuring out the two different use cases of having decorators that do not take any additional parameters and decorators that act like factories and will take additional parameters. in turn returning the actual decorator function? And furthermore, according to the API documentation of these decorators, (not) using these with |
@silkentrance You gave an example which my proposal doesn't affect at all. My proposal affects only this case: class Foo {
@decorator foo() {}
} While your example above will work without changes |
@silkentrance and answering your question "What's so comlicated...". I'm using framework Angular2. Which has Right now I should open a documentation and spend a time, because it's not obvious. With my proposal I don't need spend extra time, because it doesn't matter. So here is a reason: to make things simpler. |
@alexbyk you can't be serious. is this a prank? @loganfsmyth please see for yourself. |
@silkentrance I don't understand why are you asking. I am serious. And I can assure you, simplifying things, trying to avoid future mistakes ans saving time is a serious stuff for every developer. And even saving 2 symbols in the syntax worth this discussion. I gave a good reason in the comment above. My proposal will save 30 seconds for you if you ever try to write Angular 2 application using i'ts Injectable decorator. Don't you thing 30 seconds for every similar case is enough to not close this discussion? |
@silkentrance of course i have. And I'm not going to ask "have you?" because spamming this discussion with worthless questions won't make difference. |
@alexbyk I disagree 😀 see for example jayphelps/core-decorators#51 for a decorator that uses some kind of "meta programming" to get things working, limited as they may. Now, how would you realize your proposal with that decorator? Well, it is rather easy, but still, it requires additional and mostly redundant effort on behalf of the developer of that decorator. The emphasis here lies on redundant effort. |
@silkentrance I made a source code review of core-decorators repo and can say "it contains a lot of bugs". I also made a step-by-step guide which describes where to find those bugs here https://github.com/alexbyk/decorators-flaw . I assure you after reading this guide step-by-step you will understand the whole picture and will go to teach maintainers of core-decorators repo. So give it a try. |
@loganfsmyth not (necessarily) disagreeing with your other points. I've barely glanced at the thread, but:
getters and setters? |
Is this a duplicate of #23? |
@alexbyk For what it is worth, you are definitely putting some effort into this. So, 50% of the developer community do not get how things work with decorators, which is you. And the other half of the developer community which is 99.9999%, so far has not even chimed in on this prank. And, seeing your example REPL, sorry REPO, I must say that you did a good job. But when it came to 2.b, you failed miserably, again, understanding how decorators works. @loganfsmyth sorry, but this cannot go unpunished... |
@silkentrance What punishment? All I see here is you calling this person an idiot instead of responding with a reasoned response. This entire discussion is useless, you've convinced yourself that they can't possibly be on your level and are being extremely disrespectful. You disagree, we get it. |
@silkentrance I think you should relax. As far as I can see you still don't get the original problem. I just pointed you that the library you mentioned above contains bugs and is more complex that it should be (complex means "bad quality, a lot of messy code"), and can be use as a big +1 for my proposal. But looks like you don't see it. Did you read my guide to the final part? Because it's not "How to write..", it's "Hot you shouldn't write..." guide. Anti-guide. And you are now in the 3) phase (because your were using core-decorators which is exactly the 3) phase). When you get to the Final) you will understand how wrong you were. So instead of blah-blah-blah try to work harder and become smarter. I'm sure ur a good guy but right now you have some problems that cloud your mind. And if you want to continue this conversation: "think twice, then write (order matters)" |
@alexbyk I think my feeling on all of this is that we could argue about this all day. I agree, it's not necessarily obvious that you need parens on some decorator functions because some decorators are actual decorators and some are decorator factories. It seems like something people will just need to learn, or library developers should keep this kind of thing in mind. People will learn. Referencing a value and calling a function are different things, that is how JS behaves, introducing a difference makes the language inconsistent and just introduces the opposite problem. If I write code like this that conforms to your proposal:
and I decide to refactor that code with a wrapper function
I suddenly need two pairs of parens on |
this variant is invalid according to my proposal. only this one is valid:
And the usage of it doesnt change. So this code will work as expected That's the point. If we have only one variant of writing decorators, authors will NEVER need refactor the code and never make errors |
@alexbyk your valid example is the same as mine. I get your example, that is why I went with one more level of nesting. If you nest twice, you end up going from no parens to two sets of parens. |
@silkentrance Do you mind to write some simple exercise? It will be simple, and contains 3 steps. I assure you will understand all by yourself after step 3. Try to write a class decorator that adds "greet" method to the class (write "Hello" to the console) and can be used this way. And say when you're ready (show your code) var foo = new Class();
foo.greet(); // Hello. |
@loganfsmyth yes, my valid example is the same as yours. But I don't understand what do you mean about nesting twice? Because you won't need 2 parentheses: A decorator as you wrote it is valid function makeDecorator(){
function decorator(){
return (target, prop, desc) => {};
}
} And usage after my proposal: all these variants are valid: class Foo {
@makeDecorator() // emtry args
method(){}
} And this too class Foo {
@makeDecorator // no parens means (), so this variant is equalt to the variant above
method(){}
} And when we want to pass some arguments, we don't need to refactor 'makeDecorator', it just works as expected class Foo {
@makeDecorator(1,2,3) // some arguments
method(){}
} So we would better use just P.S.: Right now you can archive the same result by examining the arguments. Like the most of libraries does. Stop. You can't. So the most of libs right now contains a bug |
@loganfsmyth I made an example when you should use 2 pairs of parens, it works the same way with my proposal or without, and doesn't make sense for me . Also this function isn neither decorator, no decoratorFactory. Is that what you mean? But my proposal doesn't affect (2)decoratorFactory, it only removes (1), so all decorators written the "right way"(2) will be working without changes. Decorators written like this:
works right now, but become invalid and should be rewritten according to my proposal |
@alexbyk My argument is not that it doesn't work, I'm just saying that you are trading unexpected behavior at one side for unexpected behavior on the other side. Rather than having to think of "do I use parens on this decorator or not" in the current proposal, in my example you have to remember "do I use one set of parens or two sets". |
@loganfsmyth I am not calling anyone stupid. But, see, your´s and possibly also my explanations are rather at the meta level which maybe these folks do not get their heads around very well. So perhaps, employing a Meinungsverstärker would do the trick? JUST JOKING. |
You say
but then
says exactly the opposite. Everything about your responses and your attitude indicates that you consider yourself to be way smarter than the OP. |
@loganfsmyth can we please get back to the topic at hand? And, yes, I am having a whole lot of fun right now, trying to convince these peops that they are wrong in their initial assessment. So maybe you chime in and we both have a lot of fun... |
@loganfsmyth meta level != concrete level, so there is a distinction here. abstract such as sarcasm is not understood by people very well. |
@silkentrance Sure. I think I'd said all I've got to say. If your goal is to be a member the JS community, you're not going to get there by rubbing people the wrong way. It does not matter if you are making a joke. Projecting emotion, humor and sarcasm over the internet is difficult. Generally your comments come off poorly. By all means keep doing whatever you want, but I think you'd get better results in your discussions by more carefully choosing how you approach online discourse and by treating other users as the fallible human beings they are, rather than malicious entities. |
@loganfsmyth I think right now we're both confused.
|
@alexbyk Correct. If you add another layer of functions though
becomes
which I'm saying is confusing, because now by adding one new function, you are required to add two new sets of parens. |
@silkentrance Have you already written an answer to my exercise I asked you a few comments above? Could you show it? If you can't do it because you are so smart and thinking at meta-level only, that's ok. |
@loganfsmyth yes. That's right. But As I understand, you are against magic invocation without parentheses. It's common in coffescript, perl and some other languages, but not used in JS. But that's because you are looking at the But the big +1 to CONS is all decorators will look the same, don't need refactoring and you will be able to be used this way:
right now it's either impossible, or is buggy to implement |
@loganfsmyth Also, I don't think someone ever wants to write |
I feel like splitting this discussion between here and #23 is just coming to be confusing. Would you be willing to combine the discussions over there since it is the older issue? |
@loganfsmyth This is where sarcasms comes in. I always consider humans to be fallable, I myself am, but being fallable and just ignorant and driven by some weird ambition are two different types of things, which I wanted to point out. Regardless, you are right in your perception that #52 and #23 are the same issue, just as I pointed out in the first place. And there might be others... |
issue moved here #23 |
Right now there are 2 forms of @decorators: (1)decoratorFunction and (2)decoratorFactory, which produces a (1)decoratorFunction.
The difference for end users is only parentheses. If there are parentheses, decorator is in the 2nd form, if there are no parentheses, it's in the 1st form
The problem is: while we have 2 possible variants, it's not obvious which one to choose, and that leads to mistakes
My proposal is to forbid the (1)decoratorFunction form and consider a decorator as (2)decoratorFactory, even without parentheses
If my proposal is accepted, there will be no forgot parentheses bug.
Also, because parentheses without arguments will be always optional, the syntax will be much clearer in the most of the cases
the same, parentheses without args are optional
desugaring:
and G should be written this way:
P.S. Writing universal decorator, that examines arguments in runtime and behaves either (1) or (2) is impossible. See proofs below. Or, if you don't understand why, here is my step-by-step explanation with examples: https://github.com/alexbyk/decorators-flaw . Also please read it before pointing me to the SomeAwesomeLibrary because in the most of the cases all libraries that provide "universal/smart" decorators syntax have the same bugs no matter how popular they are. So if you library provide
@withSomething class Foo {}
syntax, it 100% has a potential bug. The same thing about attribute decoratorsThe text was updated successfully, but these errors were encountered: