Skip to content

fix(document-injection): Update to use injected document #18780

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 12, 2020
Merged

fix(document-injection): Update to use injected document #18780

merged 3 commits into from
Mar 12, 2020

Conversation

timsawyer
Copy link
Contributor

Update several classes in Material and the CDK to reference
the injected document instead of accessing the global document
and window variables. This fixes use-cases where the document
is dynamically provided. This change follows the pattern used
in several other places such as the Cdk OverlayContainer.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 10, 2020
@timsawyer timsawyer requested a review from a team as a code owner March 10, 2020 18:45
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Mar 10, 2020
@crisbeto
Copy link
Member

Others can pitch in, but I'm not sure whether this is worth the extra churn that it introduces. All of the cases that are using the global document or window are in places that require some kind of user interaction so we aren't risking breaking server-side-rendered apps.

@timsawyer
Copy link
Contributor Author

The primary point of this PR is not to support SSR. In my team's project (and potentially others), we are dynamically injecting Angular Material components into iframes. Because of this, we need to be able to configure the CDK and Material to subscribe to the correct document/window.

@timsawyer
fix(document-injection): Update to use injected document
21a7c5c 
Update several classes in Material and the CDK to reference
the injected document instead of accessing the global document
and window variables. This fixes use-cases where the document
is dynamically provided. This change follows the pattern used
in several other places such as the Cdk OverlayContainer.
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Mar 10, 2020
@jelbourn jelbourn added the G This is is related to a Google internal issue label Mar 10, 2020
jelbourn
jelbourn reviewed Mar 10, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you remove the new additions from the components' public APIs, you can run the command(s) from the api_golden_checks job to add the new API golds to the PR.

@jelbourn jelbourn added the target: patch This PR is targeted for the next patch release label Mar 10, 2020
@timsawyer
fix(document-injection): Update to use injected document
5aeed1b 
Utilize private functions for _getWindow() and _getDocument()
instead of using public getters.
@timsawyer
fix(document-injection): Update to use injected document
a0c5a68 
Update golden files to reflect API changes.
@timsawyer
Copy link
Contributor Author

Golden API files have been updated now also.

mmalerba
mmalerba reviewed Mar 10, 2020
View reviewed changes
@mmalerba mmalerba added lgtm action: merge The PR is ready for merge by the caretaker labels Mar 10, 2020
mmalerba
mmalerba approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jelbourn
jelbourn approved these changes Mar 10, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@timsawyer
Copy link
Contributor Author

@crisbeto @andrewseguin @devversion

Could you please review this change when possible?

@jelbourn
Copy link
Member

jelbourn commented Mar 12, 2020 •
edited
Loading

@timsawyer they're not required; once it has the merge-ready label, it goes into our merge queue, pending global presubmit

@timsawyer
Copy link
Contributor Author

Ok cool, thanks for the clarification!

@andrewseguin andrewseguin merged commit 204db56 into angular:master Mar 12, 2020
andrewseguin pushed a commit that referenced this pull request Mar 12, 2020
@timsawyer @andrewseguin
fix(document-injection): Update to use injected document (#18780)
350e598 
* fix(document-injection): Update to use injected document

Update several classes in Material and the CDK to reference
the injected document instead of accessing the global document
and window variables. This fixes use-cases where the document
is dynamically provided. This change follows the pattern used
in several other places such as the Cdk OverlayContainer.

* fix(document-injection): Update to use injected document

Utilize private functions for _getWindow() and _getDocument()
instead of using public getters.

* fix(document-injection): Update to use injected document

Update golden files to reflect API changes.

(cherry picked from commit 204db56)
@snyk-bot snyk-bot mentioned this pull request Mar 19, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request Mar 28, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request Apr 8, 2020
Merged
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@devversion devversion devversion approved these changes

@mmalerba mmalerba mmalerba approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@crisbeto crisbeto Awaiting requested review from crisbeto

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@timsawyer @googlebot @crisbeto @jelbourn @devversion @mmalerba @andrewseguin