Open boris-kolar opened 6 years ago
Does this remove protected too ?:)
Perhaps there is also need for the opposite, adding public for all privates ?
+public [P in keyof T]: T[P];
I suppose it is very useful feature, because it allows to work better with decorators. For me it is quite common case when you create property decorator and need access to some private or protected property, but cannot get it due to current restrictions. That's why you are forced to use any
type in decorators, and it does not guarantee type-safety.
The minimal example of this kind of decorators is following:
type Public<T> = {
+public [P in keyof T]: T[P]
}
const decorate = (target: any, properyName: string) => ({
get(this: Public<Base>): any { // here instead of `Public<Base>` I use `any` for now
return this.props[propertyName];
},
set(this: Public<Base>, value: any): void {
this.props[propertyName] = value;
},
});
class Base {
private props: {[key: string]: any} = {};
@decorate
public property: string = '';
}
I could really use this too.
What kind of "feedback" is needed when it says "Awaiting feedback"? And by how many?
Without something like this, is there anyway to express a similar behavior with what there is currently available in TS? Because I would like this as well, for use in decorators.
this could also be useful for testing, to mock private methods or properties that normally should not be exposed, for example:
beforeAll(() => {
const serviceA = getServiceA(...) as Public<ServiceA>;
serviceA.tokenValidityMs = 1000;
jest.spyOn(serviceA, 'somePrivateMethod').mockResolvedValue(42);
});
I would like to see this arrive in TypeScript as well. I have a ton of unit tests that force me to cast my objects as any
in order to test their private stuff. This would give back full type safety within my tests.
@TwitchBronBron in general it's considered bad practice to test private state. Only the public API surface should be tested anyway.
@ffMathy of course it's a bad practice, we all know it. But sometimes you have a field that logically should be private, with one exception. A hack is much simpler than refactoring for just one special case.
yeah, I think there should be a way to use private fields, but it should be always clear when it's used, so people don't overuse it.
Rationale / example case when it's "not that bad" practice: in tests, to temporarily change a service's variable that controls some of its behavior. It makes sense for variables that are normally private since they're set from process.env / other external sources on class instantiation, cannot change at runtime, and there is no reason to expose them to other services. Currently you can only do that with (service as any).someSetting = ...
, but then it's not typechecked, so you won't notice if you make a typo or the service changes.
Is this being looked at at all? It would be nice to see this feature implemented.
Bumped into this when I was searching for a solution for a similar problem +1
Adding and removing modifiers is definitely a must
This could also help with workarounds to the lack of an override
keyword like this one which currently only works on public
members (when it would ideally work on protected
members too).
Looks like this would also help with my use case.
Currently, in Angular the @Inject(SOME_TOKEN)
decorator of constructor params is totally type-unsafe, despite having exact type information in SOME_TOKEN. I believe the reason is that it's not possible to implement a type-safe decorator for a private property initialized in a constructor like this:
constructor(@Inject(TOKEN) private myProperty: MyType)
So, @RyanCavanaugh, after 2.5 years with "Awaiting More Feedback", could you please removed this label? This feature is really a much needed thing, especially in testing.
@KostyaTretyak with only 15 upvotes in 2.5 years, removing the label would imply closing this based on how often people are encountering it
@RyanCavanaugh, as for me, you don't even have to ask anyone about it, it's obviously a necessary thing. But maybe you know how, for example, to test protected or private properties of objects?
I test private or protected properties the same way I interact with them in normal usage - through their public interface, because private/protected members are an implementation detail that shouldn't be exposed to testcases.
@RyanCavanaugh It's more than just testing... I would like access to private/protected properties for ORM reasons. Using DDD patterns for Domain classes sometimes requires using public methods to set these private properties which are then saved into and back from the database. Using query patterns for these private properties would require creating a separate class to map the private properties to making it really painful and redundant (not to mention its not typesafe).
On a side note (and I can't find the issue on this)... how many votes were required to make -readonly
possible?
because private/protected members are an implementation detail that shouldn't be exposed to testcases
Exactly. And because of this there is a need to write mocks to ignore the implementation detail. And without this feature it is quite difficult to write mocks.
@RyanCavanaugh, I have an example from real life. I want to test the patchPostDraft()
of the PostEditController
class:
@Controller()
export class PostEditController {
constructor(
private req: Request,
private res: Response<ApiResponse<any>>,
private local: LocalizationService,
private mysqlService: MysqlService,
private config: ConfigService,
private paramsService: ParamsService,
private authService: AuthService,
private emailService: EmailService,
private postEditService: PostEditService,
private utilService: UtilService
) {}
@Route('PATCH', ':postId', [AuthGuard])
async patchPostDraft() {
const { postId } = this.req.routeParams;
this.paramsService.id(postId);
const [{ previewId }] = await this.postEditService.updateDraft(postId);
this.res.sendJson({ meta: { previewId } });
const { wantReview: rawWantReview } = this.req.body as PostEditModel;
const wantReview = this.paramsService.convertToBoolNumber(rawWantReview);
if (wantReview) {
await this.emailService.sendDraftToReview(this.postEditService.ownerPubId, postId);
}
}
// ... other methods
}
Even considering that my framework has a Dependency Injection system, it is still quite difficult to write tests without the feature described in this issue. In order to ignore the implementation details of the 10 services listed in the constructor, I need to simulate their results with mocks.
How do I work with these mocks then? As mentioned in this issue, I have to sacrifice type checking:
(postEditController as any).req = { routeParams: { postId: '10' }, body: { authorshipId: 1 } };
And then, to write tests with jest, I also need to bypass type checking:
expect((postEditController as any).res.sendJson.mock.calls.length).toBe(1);
expect((postEditController as any).res.sendJson.mock.calls[0][0]).toEqual({ meta: { previewId } });
As you can see, not only is the PostEditController
instance type lost, the types that come with .mock.*
are also lost.
There is a safer way to check types, but also a more complex one. In my case, in PostEditController
I first make the mentioned properties protected
, and then in its mock I make them public
.
But obviously, this is a workaround, and developers have to create protected
properties where private
properties are actually needed.
@KostyaTretyak in your example req
is passed as an argument to the controller's constructor. Looks like you can instantiate the controller directly, providing whatever mock implementations you want as arguments. Or you can use your DI container to instantiate it for you. In both cases it's not really required to access the private property of the controller.
Workaround: there is a way to access private fields with myObject["myPrivateField"]
syntax.
Workaround: there is a way to access private fields with myObject["myPrivateField"] syntax.
Thats not type safe. If you change your code you wont get error at compile time.
Every major languages have a way to access private/protected methods/variables.
It is nice to avoid it and only interact/test the class using public methods/variables but let's face that it can be a pain in the ash.
For that this feature would give a nice a solution with compile time checking. The programmer's responsibility to test the class properly (as it is always).
I think the strongest reasons for having this feature are more practical than theoretical.
@matepek Actually the someObject["privateField"]
access is type safe: Playground link
Cool, didn't know that. I guess it requires some typescript flag to be enabled because I weakly remember trying something like this before. Thanks.
Actually the someObject["privateField"] access is type safe.
But during refactoring (rename property via F2
), the value in the expression someObject ["privateField"]
will not change (at least in VS Code and in TypeScript's playground).
@ahejlsberg, @DanielRosenwasser, @RyanCavanaugh, please guys, let's at least discuss this issue. Should the number of upvotes be decisive in whether or not to implement this feature? I am deeply convinced that this feature is essential if we want to use the type safe way.
If you believe users can work without this feature, please explain how to achieve this without workarounds. I gave a specific example above. Although I can still transmit mocks via DI, then I won't be able to use one of the most popular frameworks - jest - to control how certain private methods are called.
I much prefer the internal
type proposal (https://github.com/microsoft/TypeScript/issues/5228) instead of mutating access modifiers.
@milesj the internal
type proposal doesn't quite satisfy this issue, because there are valid situations where properties should be private
even within the same module.
please guys, let's at least discuss this issue.
@KostyaTretyak given the current feedback and our general belief that private
does in fact mean private
, this is not going to happen.
@RyanCavanaugh
our general belief that private does in fact mean private
Except that there are ORM's that use private variables to map back and forth to databases. This means there needs to be "ways" to query on this data (using the private properties). I don't think you can carte blanche say just because this is the belief, that it's practically being used this way all the time. I would ask that you reconsider the use-cases.
@UTGuy Sounds like those ORMs should not be using private
then?
@milesj The point was that you can't practically force library creators to follow the "private means private" belief. These libraries already exists and more are being created. They are out there solving real world problems.
The problem is that there is already a precedent set by allowing "-readonly" modifiers.. and somehow there's been a line drawn in the sand over "private".
private
does in fact meanprivate
@RyanCavanaugh, how about readonly
? It is not mean readonly
?
Types of private
fields are removed during .d.ts
emit, so there'd be a huge gap in this when crossing project boundaries, so this would be really problematic in terms of surprising behavior when used in earnest. That isn't true of readonly
.
This means there needs to be "ways" to query on this data (using the private properties).
There will always be things you can do that the type system can't undo for you; the notion that there has to be syntax to do anything you can think up isn't based in reality.
So far the use cases I've seen are related to using decorators together with private properties. Maybe the right question to ask should be "how do we make a decorator aware of the type of a private property it applies to?", rather than "how do we remove private modifier?"
Any thoughts?
@RyanCavanaugh
the notion that there has to be syntax to do anything you can think up isn't based in reality.
Nobody is saying that.
This means there needs to be "ways" to query on this data (using the private properties).
... meant, it's gone into the database using private property names, and we will have to query on those private property names. The hope was there could be a type-safe way to do this using the original class/interface. Regardless of whether -private
came to be... it still will have to be queried this way.
I will just have to create a separate interface to query with. It will have public versions of the same private properties of the other class/interface. If that's how it has to be... it's just unfortunate there is no tie between the 2 classes/interfaces
Your original statement was that "private
does in fact mean private
". But as you've stated, its actually more like... "the implementation of definition files prevents us from using -private
, which is different than that of -readonly
". -Which is fine.
private does in fact mean private
It depends on context.
Does private mean private for JSON.stringify? Fortunately, it does not. (And it's a real pain that JS hides #private
fields even from JSON.stringify).
There are tasks when access to private properties is required, and all those tasks can be categoried as (1) mapping and (2) testing. For testing, it's in general okay to use workarounds such as ["fieldName"]
syntax, but mapping is different.
What I mean by mapping is serializers, ORMs and so on: in general, serialization/de-serialization and ORMs do the same thing: take an object (including all inner objects), convert its internal representation to a different format (does not really matter whether it's a JSON-encoded string or tuples and columns in a relational database), and do the reverse operation (again, does not really matter what's the source of properties for hydration) and get the same objects back.
In languages like Java, C# or PHP, the usual approach is to use Reflection to implement mapping and access private/protected properties. Another approach is to make objects serializable or relational-mappable by themselves, which is considered as a bad practice and an antipattern, and a violation of SPR: such approach mixes domain and infrastructure levels, and introduces tight coupling to specific infrastructure (you can find a lot of articles criticizing Active Record).
And it's a real problem right now. Take a look at the Mikro-ORM library, which is I'd say the first ORM written in Typescript which tries to properly implement DataMapper and UnitOfWork patterns. The author did a great job to ensure type safety, but there's an unfortunate compromise that had to be made because of this exact issue we're discussing here: all properties need to be public, even when it's undesirable - and, if you're following principles of object oriented development and doing proper incapsulation of domain models, it's never desirable. A properly designed domain model keeps itself consistent, and that's something you cannot do unless its properties are private.
And, btw, Reflection-based approach is not really compile-time type-safe. Typescript has an opportunity to do much better here!
So far the use cases I've seen are related to using decorators together with private properties.
Not really. Take a look at how entity schema is defined in mikro-orm. Lack of "private" typing requires to (a) make all the entity properties public (and lose the consistency), or (b) describe private properties in a separate interface with the same properties but being public, and use weird casting like entityClass as unknown as Class<EntityPropertiesInterface>
as a workaround - and you need to pay a lot of attention to keep this intermediary interface in sync.
(And, while mikro-orm supports decorators, decorator-based schema definition is considered a bad practice because, again, it mixes pure domain logic with details of infrastructure implementation).
Another example is custom serialization: say we want to serialize a plain object to something like msgpack, and then deserialize it back with no typing loss. There's currently no way to make it type-safe, and the only way to actually make it type-safe is typing for private properties.
So there's real and immediate need for this feature.
Can it be abused? Of course. Like <any>
can be abused, or ["fieldName"]
, or any other powerful but potentially unsafe language feature - like Reflection in Java. But, I believe, one of the reasons of success of Typescript is that it's practical. And, in practice, it's really needed to be able to implement a good ORM or something like a C# AutoMapper.
Types of private fields are removed during .d.ts emit
I actually think that it'd be desirable behavior. When I'm mapping, serializing or testing my own stuff, I know the internals and I know what I'm doing. I cannot imagine a reasonable use case for doing that for vendor-imported code which is supposed to hide its implementation details (which can be changed even in a minor release).
with only 15 upvotes in 2.5 years, removing the label would imply closing this based on how often people are encountering it
Nothing unexpected here. This feature is mostly needed by ORM/serialization/testing library developers. A lot of similarity with reflection here: a few people actually need it, but those few, given such a feature, create powerful tools used by many.
@kbaryshnikov I agree with most of your post (and thumbs up for writing such a response), but, I'd like to point out a few things...
all properties need to be public, even when it's undesirable
In the context of Mikro-orm and Decorators this is not the case... properties can be private. The schema is discovered through decorators instead of declared up front.
And, while mikro-orm supports decorators, decorator-based schema definition is considered a bad practice because, again, it mixes pure domain logic with details of infrastructure implementation
I disagree... Decorators are part of what's called Aspect-oriented programming, and fundamentally abstract away the database in a declarative way. AOP can be seen not just in Typescript/Javascript, but in C# as well (and I'm sure many other languages).
@kbaryshnikov good point. Data mapper is a good example.
One could argue that this particular implementation (mikro-orm) is not really type safe anyway. Unless I missed the point, in the schema you have to specify a runtime type of a property - and there is no compile time guarantee it matches the actual class property definition.
I would say such use case would benefit from a full-featured reflection api in Typescript (which is currently only achievable with decorators and not as an independent first class concept)
@amakhrov
Unless I missed the point, in the schema you have to specify a runtime type of a property - and there is no compile time guarantee it matches the actual class property definition.
Mikro-orm IS type-safe using Decorators on the Domain model. You would use the Domain model in the query, as Mikro-orm will make the query type-safe based on that. The only place it's not type-safe is private (decorated/persisted) properties... as you cannot query on them. This is my problem, and the reason I would like to see this feature.
@UTGuy , right, my comment was about defining schema via EntitySchema
, without decorators (in response to @kbaryshnikov). I agree that with decorators it has desired type safety, except for private
.
I just wanted to know more use cases beyond decorators where removing the private
modifier would be beneficial.
Decorators are part of what's called Aspect-oriented programming, and fundamentally abstract away the database in a declarative way.
Sure, but that approach does not really abstract domain from infrastructure. The idea of DDD is domain modeling first, with persistence layer completely being out of scope of domain level - even the fact that a relational database is being used for persistence should be outside the scope. Ideally, one could even switch between different ORMs without changing a single line in domain entities code.
I can agree that AOP approach has its benefits, but I believe the choice should be made by programmer, and not enforced by language design.
in the schema you have to specify a runtime type of a property - and there is no compile time guarantee it matches the actual class property definition.
Those types are mappings to SQL types, and, of course, there's no real compile time guarantee for that (and I doubt it's even possible to do without dependent types). But it tries to do what's possible: queries in find* methods are (moslty) type-safe, take a look at Query<T>
for example.
I just wanted to know more use cases beyond decorators where removing the private modifier would be beneficial.
I believe it's a good idea to think of a type-safe Automapper as a general case. All the specific cases like ORM, serialization, etc are basically mappers with extra steps.
For a specific use case, imagine something like Apache Avro, but with a type-safe schema definition in Typescript.
Sure, but that approach does not really abstract domain from infrastructure
I'm not sure what you mean by this. I assume its the way your thinking this is implemented. My infrastructure layer sits outside of the domain layer. The purpose of Decorators (in this case) is to not introduce infrastructure into it, only annotate how to map the domain to the w/e database your using.
Ideally, one could even switch between different ORMs without changing a single line in domain entities code.
The only thing you would change is swapping out the Decorators used.
I think we just disagree on the level of intrusion to the Domain that is OK to have.... I'm personally fine with only having decorators as a mapping because it keeps it isolated. If you change your domain, you would have to change how you map it to the database regardless. It's just nicer to keep these things close to each other... hence the AOP.
The only thing you would change is swapping out the Decorators used.
Exactly, and that means there's a static dependency. What if I'm switching from database A to database B, and, for a transitional period, I need to persist to both databases? Or may be I need to use both databases for some reason?
With external schema definition there's no coupling at all.
I think we just disagree on the level of intrusion to the Domain that is OK to have
Yeah, it seems so. So let's agree to disagree here. :-)
In my use case, I'm looking to create a unit test to that describes how some private internal state is initialized by a constructor.
Here's a toy example which hopefully does enough to show what I'm trying to do:
class Thing {
private internalState: number
constructor(initialState: number = 0) {
// some sort of logic in here...
this.internalState = initialState > 100 ? 100 : initialState ;
}
}
test('initialization', () => {
const thing = new Thing(1000);
expect(thing.internalState).toEqual(100); // Error: Property 'internalState' is private
})
Basically, I want my unit test to describe whatever logic exists in my constructor and some of that logic describes how private state is initialized. I can test this in indirect ways through my public API, but I want to cut right to the chase in the tests for the sake of clarity.
In my opinion @Lodin has made a solid suggestion how this could be implemented without breaking any existing behavior.
type Public<T> = {
public [P in keyof T]: T[P]
}
This syntax really fits with the Partial
, Required
and Readonly
Utility Types. This would just force all members to be public.
type Partial<T> = {
[P in keyof T]?: T[P];
};
type Required<T> = {
[P in keyof T]-?: T[P];
};
type Readonly<T> = {
readonly [P in keyof T]: T[P];
};
Besides testing and serialization, decorators are a valid point where one wants to be able to reference the private members as well.
To provide another "real world" example:
I wanted to write a decorator which would wrap methods and cache their results depending on the arguments. Since those arguments include private members of the class you would need to supply them to the decorator.
type Members<
Instance extends object,
Keys extends keyof Instance = keyof Instance
> = Keys extends (Instance[Keys] extends (...args: any[]) => any ? never : Keys) ? Keys : never;
function cache<Instance extends object>(
reliesOn: Members<Instance>[] = []
) {
return function (
target: any,
propertyKey: string,
descriptor: PropertyDescriptor
) {
descriptor.value = function (...args: any[]) {
// cache logic
}
}
}
class Example {
public publicProp: number = 1;
private privateProp: number = 2;
@cache<Example>([
"publicProp",
"privateProp"
])
someFunction(a: number, b: number) {
return this.publicProp + this.privateProp + a + b;
}
}
The publicProp
will be suggested and does not raise an error but privateProp
will. There is no workaround for this usecase. The only other way would be to just accept strings, which would render the typechecking inert for the decorator and force the user to manually keep the identifier in sync.
The only other way would be to receive the used properties as arguments which have their default value set to the value of the property. But this would change the signature of the method, which is not wanted.
@RyanCavanaugh
I believe there are other reasons for the lack of interest in this issue. For example, I had a hard time finding this issue in the first place. How much of an interest would the typescript team require to bring this feature to the next stage? Is there anything we can do to help to archive that?
[Feature suggestion] How about this (for Typescript 2.8+):