fix(overlay): only clear duplicate containers from different platform #17006

crisbeto · 2019-09-08T11:12:03Z

Currently we clear all overlay containers when we create a new one as a way to avoid duplicate content coming in from the server. Our current approach is a little too aggressive, because it can pick up containers from different apps. These changes add an extra attribute to the container so that we can determine which platform it's coming from.

Fixes #16851.

crisbeto · 2019-09-08T12:45:09Z

src/material-experimental/mdc-select/harness/select-harness.spec.ts

@@ -63,6 +68,11 @@ describe('MatSelectHarness', () => {

 /** Shared tests to run on both the original and MDC-based select. */
 function runTests() {
+  afterEach(() => {
+    // Angular won't call this for us so we need to do it ourselves to avoid leaks.
+    overlayContainer.ngOnDestroy();


We need these changes, because the changes to the container revealed that the tests here are leaking overlay containers. I've fixed it for all harnesses in #17007.

src/cdk/overlay/overlay-container.ts

crisbeto · 2019-09-13T21:12:17Z

Addressed the feedback @jelbourn.

jelbourn

LGTM

ngbot · 2019-09-13T21:22:41Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
status "ci/circleci: ivy_test" is failing
status "ci/circleci: tests_saucelabs" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

zawislakdawidj · 2019-10-30T23:21:30Z

Any updates on this? I'm seeing issues due to #16851 and verified that this PR resolves them.

jelbourn · 2019-12-04T15:17:43Z

Looks like this has 80+ failures inside Google for a number of projects. Will require further debugging

crisbeto · 2019-12-15T12:49:06Z

I'm bumping the priority on this, because we keep getting reports about it.

jelbourn · 2020-01-23T22:53:13Z

I spent a bit debugging this. Turns out that people have been unintentionally depending on the current behavior in their unit tests in that it guarantees that there's exactly one overlay container. With the changes in this PR, overlay containers aren't cleaned up between specs, so tests just start failing whenever they expect something about a particular overlay container and get a different one (one test I looked at had 21 overlay containers). This is pretty widespread; the change breaks 170 test targets.

I feel like the real solution would be to introduce something like OverlayTestingModule that automatically fakes/clears the overlay container, but I'm open to other ideas.

crisbeto · 2020-01-24T06:25:23Z

That's interesting, considering that we weren't clearing the overlay containers for at least two and a half years. I'm pushing a change that'll clear them if it detects that it's running inside in a testing context with a TODO to remove it once we have an overlay testing module.

Currently we clear all overlay containers when we create a new one as a way to avoid duplicate content coming in from the server. Our current approach is a little too aggressive, because it can pick up containers from different apps. These changes add an extra attribute to the container so that we can determine which platform it's coming from. Fixes angular#16851.

…#17006) Currently we clear all overlay containers when we create a new one as a way to avoid duplicate content coming in from the server. Our current approach is a little too aggressive, because it can pick up containers from different apps. These changes add an extra attribute to the container so that we can determine which platform it's coming from. Fixes #16851. (cherry picked from commit 29eec77)

…angular#17006) Currently we clear all overlay containers when we create a new one as a way to avoid duplicate content coming in from the server. Our current approach is a little too aggressive, because it can pick up containers from different apps. These changes add an extra attribute to the container so that we can determine which platform it's coming from. Fixes angular#16851.

angular-automatic-lock-bot · 2020-02-27T00:03:17Z

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.}

crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Sep 8, 2019

crisbeto requested a review from jelbourn as a code owner September 8, 2019 11:12

crisbeto assigned jelbourn Sep 8, 2019

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 8, 2019

crisbeto force-pushed the 16851/duplicate-containers-platform branch 2 times, most recently from baf8cc8 to 1b30154 Compare September 8, 2019 12:43

crisbeto commented Sep 8, 2019

View reviewed changes

crisbeto mentioned this pull request Sep 8, 2019

test(material-experimental): harness tests leaking overlay containers #17007

Merged

jelbourn reviewed Sep 12, 2019

View reviewed changes

src/cdk/overlay/overlay-container.ts Show resolved Hide resolved

crisbeto force-pushed the 16851/duplicate-containers-platform branch 2 times, most recently from dca378f to f11389b Compare September 13, 2019 21:07

jelbourn approved these changes Sep 13, 2019

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 13, 2019

crisbeto force-pushed the 16851/duplicate-containers-platform branch 2 times, most recently from 539c6a9 to ea785ba Compare September 28, 2019 11:20

jelbourn added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Dec 4, 2019

crisbeto force-pushed the 16851/duplicate-containers-platform branch from ea785ba to 299e423 Compare December 15, 2019 12:48

crisbeto added P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Dec 15, 2019

crisbeto force-pushed the 16851/duplicate-containers-platform branch from 299e423 to 97c67f1 Compare December 27, 2019 14:25

crisbeto force-pushed the 16851/duplicate-containers-platform branch from 97c67f1 to e5c93fc Compare January 24, 2020 06:37

jelbourn removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jan 27, 2020

jelbourn merged commit 29eec77 into angular:master Jan 27, 2020

aecz mentioned this pull request Feb 3, 2020

using single-spa and angular8 to develop micro front-end application,the cdk overlay is not working correct #16972

Closed

crisbeto mentioned this pull request Feb 17, 2020

Issue #18519: cdk overlay container removed fix #18522

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(overlay): only clear duplicate containers from different platform #17006

fix(overlay): only clear duplicate containers from different platform #17006

Uh oh!

crisbeto commented Sep 8, 2019

Uh oh!

crisbeto Sep 8, 2019

Uh oh!

Uh oh!

crisbeto commented Sep 13, 2019

Uh oh!

jelbourn left a comment

Uh oh!

ngbot bot commented Sep 13, 2019

Uh oh!

zawislakdawidj commented Oct 30, 2019

Uh oh!

jelbourn commented Dec 4, 2019

Uh oh!

crisbeto commented Dec 15, 2019

Uh oh!

jelbourn commented Jan 23, 2020 •

edited

Loading

Uh oh!

crisbeto commented Jan 24, 2020

Uh oh!

angular-automatic-lock-bot bot commented Feb 27, 2020

Uh oh!

Uh oh!

fix(overlay): only clear duplicate containers from different platform #17006

fix(overlay): only clear duplicate containers from different platform #17006

Uh oh!

Conversation

crisbeto commented Sep 8, 2019

Uh oh!

crisbeto Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crisbeto commented Sep 13, 2019

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented Sep 13, 2019

Uh oh!

zawislakdawidj commented Oct 30, 2019

Uh oh!

jelbourn commented Dec 4, 2019

Uh oh!

crisbeto commented Dec 15, 2019

Uh oh!

jelbourn commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto commented Jan 24, 2020

Uh oh!

angular-automatic-lock-bot bot commented Feb 27, 2020

Uh oh!

Uh oh!

jelbourn commented Jan 23, 2020 •

edited

Loading