-
Notifications
You must be signed in to change notification settings - Fork 105
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
Parameter decorators #47
Comments
+1 to leave for follow on |
Angular is heavily using parameter decorators as part of the dependency injection capabilities of the framework. Is this feature still planned as a follow up proposal? What's the plan for the intermediate state when browser vendors will support class decorators but will not have parameter decorators support? Although this is probably a concern mostly for compilers, I believe it's a topic we should discuss. |
@mgechev I believe that parameter decorators will likely be able to desugar into method (or function) decorators. So tooling can do exactly that desugaring when we are in this intermediate state. |
May I ask why? How would libraries like Angular and TypeGraphQL function without parameter decorators? |
"There exists a library which would use this feature" is not, in itself, sufficient reason to add a feature. For a syntactic feature to be worth adding there needs to be an affirmative case that it would make some very broad class of problems significantly easier or clearer. I don't think parameter decorators meet that bar. It has not been my experience that parameter decorators make code clearer at all, in fact; most code that uses them (yes, including Angular) seems to me like it would have been clearer if it had been written in a different way entirely. (I think the ecosystem bears this out.) Perhaps I'm wrong, and on further discussion we'll determine that there are actually many cases where parameter decorators are necessary to clearly express intent. I look forward to that case being made. But right now, I have not seen many cases where I think code is or would be expressed more clearly with parameter decorators. |
-There exists a library which would use this feature
+There exist multiple libraries which do use this feature I started a list here, and there are already 8 projects using property decorators, some of them very popular. The document can be edited by anyone, so feel free to add more.
I'm sure they would be interested in your suggested alternatives. 🙂 |
I think it would be good to discuss the use cases for parameter decorators, not just the existing libraries that use them. As @bakkot has noted, just the existence of usage in the ecosystem is not really enough to motivate a feature. So far, there are two concrete use cases for parameter decorators:
Definitely would like to see if there are more, these are somewhat motivating but DI is fairly divisive in terms of motivation and validation can be done in other ways (e.g. on methods/functions directly), though it would be less ergonomic. |
In my opinion, there are several use cases for parameter decorators. Some of them are
|
Can you quantify that? |
@joelday when it was raised in committee, there were multiple members who expressed that DI was not a very strong motivating use case on its own, and that they would prefer there be more use cases to justify the feature. We haven't revisited the discussion since then as I've been focused on landing the main decorators proposal first. When that's complete, I intend to dig into the concerns behind parameter decorators more thoroughly. |
@pzuraq Thanks for the clarification. The way you worded it gave me the impression that personal bias for/against DI itself was a factor. |
@pzuraq I'd like to also submit that the entire Visual Studio Code codebase makes use of them for DI, and quite successfully so. This isn't to suggest that a single codebase should influence the direction of standards, but that libraries and their usage alone aren't necessarily the full picture. |
@joelday yes, I am aware, as are most members on the committee. Usage of pre-stage-4 transforms or features is generally not seen as strong motivating example, the logic being that the feature should make sense on its own merits and should not generally be advanced just because it was adopted by a large organization, community, or codebase. There is a strong history of pushing back against accepting existing APIs in my experience, some examples include Promises (the final API differs in key ways from community libraries that were popular at the time), classes and class fields ( Personally I am a pragmatist and am usually more happy to accept the existing community developed APIs where possible, but when working in committee and trying to advance things I generally find I have to work with a large number of different viewpoints to come to a consensus, and sometimes that means going in a different direction. |
DI can be achieved with current decorators using a different format: @injectable
class Foo {
@inject([A, B])
someMethod(a, b) {}
} |
@trusktr Method injection support isn't good enough. The idea is to support both automatic constructor injection and manual injection in-through the constructor. This allows class instantiation to be decoupled from dependency resolving and also for classes to be unit tested easily. @Injectable(AppGraph)
class ConstructorInjection {
constructor(serviceA?: ServiceA, serviceB?: ServiceB);
constructor(@Inject() private serviceA: ServiceA, @Inject() private serviceB: ServiceB) {}
}
// In your production code, instead of this:
new ConstructorInjection(new ServiceA(), new ServiceB());
// You can do this:
new ConstructorInjection();
// While still being able to easily unit test by passing mocks in a sane manner
const uut = new ConstructorInjection(mock<ServiceA>(), mock<ServiceB>()); |
Absolutely agreed. I noticed that the last meeting notes are from 2019. Do you have anything more recent? |
I would refer to the TC39 plenary notes. We have kept some notes internally, but the overhead of note taking is a decent amount and I'm stretched thin as is so I have stopped formatting/uploading them myself. The disagreements/issues I'm referring to come up in plenary however, we don't typically discuss them in the Decorators meeting. |
I was just discussing parameter decorators with @littledan, and wanted to capture some of my notes here for the inevitable parameter decorators proposal. Parameter decorators are extremely valuable for a number of scenarios, including constructor parameter injection in a DI system like the one VS Code uses. While that scenario also strongly depends on some capability for associating metadata (as is being discussed in https://github.com/tc39/proposal-decorator-metadata), I think the following approach to a native parameter decorator would solve the other side of the problem. Much like a class field, a parameter has no reified declaration for replacement. As a result, its In addition, a parameter has an initial value that is either provided by the user as an argument, via an initializer, or just function paramDec(target, context) {
target; // always `undefined`
return value => value; // can observe/replace argument passed to parameter
} As far as the provided This might look something like the following: type ParameterDecoratorContext = {
kind: "parameter";
name: string | undefined;
index: number;
rest: boolean;
addInitializer(cb: (this: unknown, ...args: any[]) => void): void;
}; We could also add additional context information related to the method being decorated, i.e.: type ParameterDecoratorContext = {
...
function:
| ClassMethodDecoratorContext
| ClassSetterDecoratorContext
| ClassDecoratorContext
}; |
Hi @littledan , I believe this is a critical import feature currently used by many TypeScript packages, such as Nest.js, TypeORM, TypeDI, TypeGraphQL, and others. Without this feature, the Stage 3 decorator cannot effectively replace the existing TypeScript decorators for this usage. Please prioritize this feature to facilitate a smoother upgrade for more packages transitioning to the Stage 3 decorator. Thank you very much. |
See tc39/proposal-decorators-previous#45
Leaving for v2.
The text was updated successfully, but these errors were encountered: