Skip to content
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(platform-browser): remove style nodes on application destroy #13744

Closed
wants to merge 2 commits into from

Conversation

DzmitryShylovich
Copy link
Contributor

Closes #11746

@@ -46,5 +46,14 @@ export function main() {
ssh.addStyles(['a {};', 'b {};']);
expect(doc.head).toHaveText('a {};b {};');
});

it('should remove style nodes on destroy', () => {
ssh.addStyles(['a {};']);
Copy link
Contributor

Choose a reason for hiding this comment

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

would this work if 2 components add the same style ?
It would be added only once to the head but should not be removed when only one of the cmp is removed.
This probably requires some kind of ref counting I don't see in this PR code.

And also there should be a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would this work if 2 components add the same style ?
It would be added only once to the head but should not be removed when only one of the cmp is removed.

not sure I'm following. My understanding is: DomSharedStylesHost is a part of BrowserModule so we can only have once instance per app (root module). Thus it will contain styles for all created components. And DomSharedStylesHost will only be destroyed when root module (or platform) will be destroyed.
Even if we have multiple app with the same platform, every app will have it's own DomSharedStylesHost. Am I wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

may be I'm wrong (I haven't look at the impl details) but if you have 2 cmp with the same css

<cmp-one></cmp-one>
<cmp-two></cmp-two>

The css would be added only once.
However if you remove cmp-one, the css must style remain for cmp-two

Copy link
Contributor Author

@DzmitryShylovich DzmitryShylovich Jan 3, 2017

Choose a reason for hiding this comment

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

I agree but I don't remove styles when component is destroyed, only when the whole app/platform is destroyed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it makes sense !

@vicb vicb added area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 2, 2017
@DzmitryShylovich DzmitryShylovich changed the title fix(platform-browser): remove style nodes on destroy fix(platform-browser): remove style nodes on application destroy Jan 4, 2017
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Jan 28, 2017
mhevery pushed a commit that referenced this pull request Jan 29, 2017
@mhevery mhevery closed this in cd3901f Jan 29, 2017
@DzmitryShylovich DzmitryShylovich deleted the gh/11746 branch January 29, 2017 19:35
mhevery pushed a commit that referenced this pull request Feb 2, 2017
mhevery pushed a commit that referenced this pull request Feb 2, 2017
mhevery pushed a commit that referenced this pull request Feb 3, 2017
mhevery pushed a commit that referenced this pull request Feb 3, 2017
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@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 Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: To support HMR, some way to remove dynamically-added style elems from head
4 participants