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: add check for style tag detached and then removed - sheet in the style tag is null in this case #2707

Conversation

@newying61
Copy link
Contributor

commented Aug 7, 2019

Hi, thanks for the great work and amazing lib.

We have a special use case of React and styled-components:

  • Rendering React App inside shadow DOM
  • Unmount React App when web component tag removed from DOM (in disconnectedCallback function)

When un-mounting the app, what I found is the sheet property in <style> tag made by makeSpeedyTag() function is null, because when disconnectedCallback getting called, the style DOM node is already removed from DOM.
Because sheet is empty, currently styled-components throws an exception in sheetForTag function.

By checking the isConnected flag, we know whether the style element is detached or not.
This flag is not supported by IE11 and Edge (<76), so, it will return undefined.
undefined === false is false in JavaScript, so, should be fine for the older browsers.

I am not sure whether styled-components should consider this issue or not.
Just raise a pull request first, if this case is too special, I'll find some other solutions.
Thanks.

@newying61 newying61 changed the title fix: add check for style tag detached - sheet in the style tag is null in this case fix: add check for style tag detached and then removed - sheet in the style tag is null in this case Aug 7, 2019
@probablyup

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@newying61 isn't this needed for TextTag as well? Would be good to get some sort of test...

@newying61

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@probablyup Thanks for getting back to me. I'll have a look and add unit tests.
I thought you'll just close this pull request, as the use case is kind of special. :P

@newying61 newying61 force-pushed the newying61:fix_stylesheet_empty_when_dom_node_detached branch from 193d36a to 508e1d1 Sep 12, 2019
@shomukai

This comment has been minimized.

Copy link

commented Sep 13, 2019

@probablyup
I added the unit tests for this case.

  • The issue only happens in speedy mode (so I have to make DISABLE_SPEEDY flag to false to use speedy mode in the unit test)
  • isConnected flag is in Node type, but flow doesn't know it from HTMLStyleElement, so, it throws an error. Need to add the $FlowFixMe.

The jsdom version used by jest doesn't support shadow DOM yet.
jsdom doesn't support web component...
So, I used normal HTMLElement to mimic the use case in the unit test.

Copy link
Contributor

left a comment

LGTM! Would you mind doing the same PR for canary?

@probablyup

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

@shomukai Also one last thing, could you add a changelog entry? :)

@newying61 newying61 force-pushed the newying61:fix_stylesheet_empty_when_dom_node_detached branch from 508e1d1 to 888a249 Sep 23, 2019
@shomukai

This comment has been minimized.

Copy link

commented Sep 23, 2019

@probablyup
Thank you very much for reviewing.
I've added the fix entry to CHANGELOG.md.

Will do a pull request to Canary today.

@shomukai

This comment has been minimized.

Copy link

commented Sep 23, 2019

@probablyup
Hi, just checked the implementation of canary.
As the sheet instance is saved in CSSOMTag constructor:

    this.sheet = getSheet(element);

So, even the tag is detached from DOM, this.sheet instance still exists.
As a result, this issue doesn't exist in canary version. 👍

Just added the unit tests and raised a pull request:
#2761

Not sure we still need it or not.
Please have a look when you have time.
Thank you.

Copy link
Contributor

left a comment

Thank you!

@probablyup probablyup merged commit e3020f6 into styled-components:master Sep 23, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@newying61

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

@probablyup
Thank you very much! m(__)m
This will help us to remove the unnecessary fix code in more than 20 web apps. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.