CDK: ConnectedPositionStrategy not open for extension

[ronaldmansveld]

Bug, feature request, or proposal:

Currently the ConnectedPositionStrategy in the CDK marks its properties and methods as private, preventing subclassing in an efficient matter. If these properties and methods were marked as protected instead, developers can more easily extend these classes to add/modify behaviour.

What is the expected behavior?

The class marks its internals as protected, so it can be subclassed and extended

What is the current behavior?

The class marks its internals as private, preventing effective subclassing and extension

What are the steps to reproduce?

Not applicable

What is the use-case or motivation for changing an existing behavior?

Currently the ConnectedPositionStrategy looks at the viewport to determine what (fallback) position to use when displaying the overlay. I have a use-case where I want this to be affected by a parent-element's boundaries instead of the viewport.
The easiest way to achieve this, is to subclass the existing ConnectedPositionStrategy, redefining the apply method to look at an injected element instead of looking at the viewport.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

All

Is there anything else we should know?

I initially raised #9194 , but that one was considered too broad.
I'm willing to create a PR to go with this issue, but want to make sure I don't miss any reasons why this class is not open for extension.

[ronaldmansveld]

As there are no comments yet on this ticket, can I assume there are no reasons that would prevent me from opening this class up?

[willshowell]

#9153 might be a reason to hold off. However, as big as it is, it may take a while to land.

[ronaldmansveld]

I had a quick look, and it seems like it still uses the viewport exclusively to calculate whether an overlay would fit at a certain position. As the new class also uses private instead of protected, I will run into the same issue that I can't extend (and therefore reuse big parts of) the class to change the behaviour slightly. While it is definitely an improvement for a lot of use-cases, it can't (and shouldn't!) solve them all. As such, I feel that this ticket is still valid, and CDK classes should use protected instead of private by default, so developers can easier use the CDK and extend it for their own use cases. I'll create a PR later this week for this, even though it may get superseded by the mentioned PR, so it can still serve as an example/proof of concept for further discussion.

…

On Mon, 8 Jan 2018, 1:42 pm Will Howell, ***@***.***> wrote: #9153 <#9153> might be a reason to hold off. However, as big as it is, it may take a while to land. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9206 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATRSTK5ye4gw8Q_5OZoIRMZhXzNSypcks5tIhtSgaJpZM4RRipw> .

[jelbourn]

@crisbeto any thoughts on this?

[crisbeto]

I'm fine with making the DI properties protected since TS won't allow properties with the same name on the derived class. As for all the other internals, if we made them protected we'd have to maintain some guarantees that they won't change between minor/patch version which will make it really hard to do any kind of refactoring.

[willshowell]

It doesn't appear that ConnectedPositionStrategy would support this, but FlexibleConnectedPositionStrategy seems to rely solely on ViewportRuler.getViewportRect() for its calculations. It would follow that you could provide your own implementation of ViewportRuler that defines a custom position and size.

@crisbeto do you think that would work?

[crisbeto]

That would work, but it still implies that we won't change the way we use the ViewportRuler (however unlikely that is).

[ronaldmansveld]

Hmm, I see the issue with refactoring. When making them protected, they more or less become part of the public API as those will be the extension points. This will, however, always be an issue when trying to make things open for extension.

The best solution indeed seems to be to provide a custom implementation for the ViewportRuler (That one can be easily provided through the DI, by telling the module to use a different class in the provides), though I'm not 100% sure what implications that may have in the places where you do need the original ViewportRuler. Can you do DI overriding per class? (As in: tell one class that you'll provide ViewportRuler with a useClass:, and don't do so on another?) Or would that be per module?
Also, how would that work out with the rest of the DI hierarchy? We don't want to impact other classes that may rely on the ViewportRuler inadvertently.

I understand that solving it with DI takes it out of the realm of the CDK, but I still feel that extensibility is an important thing, opening a lots of doors that would otherwise stay closed.

[ronaldmansveld]

This may also link to some extend to #9331 : That's also a strategy that requires a different parent than the viewport.

[crisbeto]

@ronaldmansveld you can scope your DI override either to the module, or to the component that is creating the overlay.

[SchnWalter]

ronaldmansveld:
CDK classes should use protected instead of private by default

I've also bumped a few of times into issues that could have been solved easily solved by extending a default feature from CDK.

So +1 for reviewing if the protected access modifier could the the default modifier in CDK