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 an issue when re-rendering lit-html elements after rendering a non-lit-html element #5868

Merged
merged 4 commits into from
Mar 11, 2019

Conversation

ndelangen
Copy link
Member

Issue: #5852

It seems the mountNode becomes stateful after a render has occurred in lit-html.
Since storybook is changing the dom beyond lit-html, it gets "confused" and throws an error because it's unable to remove
some nodes. I guess it's trying to do reconciliation or cleanup, and it's missing state or it's state is inconsistent
with the actual dom.

By adding a extra inner root node that gets reset when switching stories we make lit-html mount correctly

What I did

I changed the render code of app/polymer to reset the dom (harder) when switching to a lit-html element.

…n-lit-html element

It seems the mountNode becomes stateful after a render has occurred in lit-html.
Since storybook is changing the dom beyond lit-html, it gets "confused" and throws an error because it's unable to remove
some nodes. I guess it's trying to do reconciliation or cleanup, and it's missing state or it's state is inconsistent
with the actual dom.

By adding a extra inner root node that gets reset when switching stories we make lit-html mount correctly
@vercel
Copy link

vercel bot commented Mar 5, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #5868 into next will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #5868      +/-   ##
==========================================
- Coverage   35.01%   34.99%   -0.02%     
==========================================
  Files         648      648              
  Lines        9480     9483       +3     
  Branches     1360     1334      -26     
==========================================
  Hits         3319     3319              
- Misses       5532     5534       +2     
- Partials      629      630       +1
Impacted Files Coverage Δ
app/polymer/src/client/preview/render.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3176058...661481a. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #5868 into next will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #5868      +/-   ##
==========================================
- Coverage   34.93%   34.92%   -0.01%     
==========================================
  Files         648      648              
  Lines        9516     9518       +2     
  Branches     1352     1352              
==========================================
  Hits         3324     3324              
- Misses       5575     5576       +1     
- Partials      617      618       +1
Impacted Files Coverage Δ
app/polymer/src/client/preview/render.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Item.tsx 0% <0%> (ø) ⬆️
addons/viewport/src/Tool.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Rules.tsx 0% <0%> (ø) ⬆️
addons/a11y/src/index.ts 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acb1555...68b6009. Read the comment docs.

@daKmoR
Copy link
Contributor

daKmoR commented Mar 9, 2019

I can confirm that this fixes #5852

any chance this can be released? or is there anything else to do?

@shilman shilman merged commit 8bc1fe6 into next Mar 11, 2019
@shilman shilman deleted the fix/lit-html branch March 11, 2019 08:18
@daKmoR
Copy link
Contributor

daKmoR commented Mar 11, 2019

woop woop thxxxx :)

@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Apr 28, 2019
@shilman shilman added this to the 5.0.x milestone Apr 28, 2019
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 28, 2019
shilman added a commit that referenced this pull request Apr 28, 2019
FIX an issue when re-rendering lit-html elements after rendering a non-lit-html element
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants