-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
fix(overlay): better handling of server-side rendering #8422
Conversation
src/cdk/a11y/live-announcer.ts
Outdated
@@ -14,7 +14,7 @@ import { | |||
SkipSelf, | |||
OnDestroy, | |||
} from '@angular/core'; | |||
import {Platform} from '@angular/cdk/platform'; | |||
import {DOCUMENT} from '@angular/platform-browser'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DOCUMENT
from platform-browser is deprecated; should use the one from @angular/common
instead
// causes TypeScript to preserve the constructor signature types. | ||
this._liveElement = elementToken || this._createLiveElement(); | ||
} | ||
@Inject(DOCUMENT) private _document: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do:
@Inject(DOCUMENT) private _document: Document|Document
It shouldn't preserve the type info for "computed" types; I think this will trick it will still giving type safety
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem to work in AoT.
deps: [[new Optional(), new SkipSelf(), OverlayKeyboardDispatcher]], | ||
deps: [ | ||
[new Optional(), new SkipSelf(), OverlayKeyboardDispatcher], | ||
DOCUMENT as InjectionToken<any> // We need to use the InjectionToken somewhere to keep TS happy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Coerce to `InjectionToken` so that the `deps` match the "shape"
// of the type expected by Angular
element.parentNode.insertBefore(this._wrapper, element); | ||
this._wrapper.appendChild(element); | ||
this._wrapper = this._document.createElement('div'); | ||
this._wrapper!.classList.add('cdk-global-overlay-wrapper'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put the !
on the assignment for _wrapper
?
this._wrapper = this._document.createElement('div')!;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS can't figure it out when assigning to a property.
Fixes multiple server-side rendering issues in the overlay that were causing errors when an overlay is opened in Universal. Fixes angular#8412.
c24e05d
to
24492be
Compare
Addressed the feedback @jelbourn. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
Fixes multiple server-side rendering issues in the overlay that were causing errors when an overlay is opened in Universal.
Fixes #8412.