fix(overlay): detach method returns undefined #7449
Conversation
googlebot
added
the
cla: yes
src/cdk/overlay/overlay-ref.ts
Outdated
| */ | ||
| detach(): Promise<any> { | ||
| /** Detaches an overlay from a portal. */ | ||
| detach(): void { |
There was a problem hiding this comment.
Change the PortalHost signature as well?
There was a problem hiding this comment.
Not sure about this. I thought that the PortalHost can still have the type in case someone wants to implement his own PortalHost.
There was a problem hiding this comment.
But since you're removing the return value from the OverlayRef, it means that even if they did, they wouldn't be able to use it with our overlays anyway.
There was a problem hiding this comment.
Portals don't have to be used specifically with overlays.
There was a problem hiding this comment.
They don't but that's the primary use case and it's something that we'll end up not supporting. Since the initial goal of the PR was to get rid of the Promise signature, consider re-adding the return and setting the return value to any?
There was a problem hiding this comment.
That would work too. In that case we would have a very unclear method signature and the return value would be for now always undefined.
Since overlays always seem to use the DomPortalHost which seems to return nothing on detach I thought we can be clear with this signature here.
I'm fine switching to any though.
There was a problem hiding this comment.
Updated as discussed on Slack.
Currently the JSDoc of the `detach` method describes that the method returns a Promise if the detaching is done. This is not the case and just `undefined` is being returned because every `PortalHost` implementation inside of the CDK is synchronously detaching the view. In the future it might be useful to change this method back to a `Promise` in favor of animations, but for now it doesn't make sense return a `Promise` because the `attach` method is also synchronous. Fixes angular#7408
devversion
force-pushed
the
fix/overlay-detach-promise-null
branch
from
0fa6ed4
to
e4a5ac8
Compare
crisbeto
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently the JSDoc of the
detachmethod describes that the method returns a Promise if the detaching is done. This is not the case and justundefinedis being returned because everyPortalHostimplementation inside of the CDK is synchronously detaching the view.In the future it might be useful to change this method back to a
Promisein favor of animations, but for now it doesn't make sense return aPromisebecause theattachmethod is also synchronous.Fixes #7408