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

Parent with Child Slot rerender bug #1968

Closed
dutscher opened this issue Oct 23, 2019 · 6 comments · Fixed by #5143
Closed

Parent with Child Slot rerender bug #1968

dutscher opened this issue Oct 23, 2019 · 6 comments · Fixed by #5143
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil slot-related

Comments

@dutscher
Copy link
Contributor

dutscher commented Oct 23, 2019

Stencil version:

 @stencil/core@1.7.4
 ---
 update: 15.11.2019
 ---
 @stencil/core@1.8.0
---
 update: 15.01.2020
 ---
 @stencil/core@1.8.5

I'm submitting a:

[x] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://stencil-worldwide.herokuapp.com/ or https://forum.ionicframework.com/

Current behavior:
Here is a demo: https://dutscher.github.io/stencilslots/

Here is the code which doesnt work from with comments

render() {
    const classes = [];

    if (this.disabled) {
      classes.push(`${this.componentName}--disabled`);
    }

    return (
      <Host class={classes.join(' ')}>
        Demonstrate slot rerender bug
        {/* this dont rerender slot */}
        <div>
          {/* remove condition: works */}
          {/* wrap with <div />: works */}
          {this.disabled && !!this.tag && (
            <div class="tag" innerHTML={this.tag}/>
          )}
          {/* remove div: works */}
          <div>
            before slot&gt;
            <slot/>
            &lt;after slot
          </div>
        </div>
      </Host>
    );
  }

The <slot /> renders first very well, when a rerender in the parent happens, the <slot /> going away. There is a ugly workarround, you can put a <div />. But logical you dont need this <div/>.
https://github.com/dutscher/stencilslots/blob/master/src/components/parent-workaround/parent.tsx

I got a scenario there i need more than one <div />.

I guess its a vdom diff problem.

Expected behavior:
TSX works without extra empty <div /> and the <slot /> is rendered after parent is rerendered.

Steps to reproduce:
https://dutscher.github.io/stencilslots/

Related code:
https://github.com/dutscher/stencilslots/blob/master/src/components/parent-broken/parent.tsx
https://github.com/dutscher/stencilslots/blob/master/src/components/parent-workaround/parent.tsx

Cheers and thanks for the attention :)

@NKBelousov
Copy link

Just confirmed this myself

@dutscher have you found any workaround?

@dutscher
Copy link
Contributor Author

dutscher commented Jan 15, 2020

@NKBelousov the worked workarround was, to add enough

or use booleans in your jsx conditions.

I updated the master with 1.8.5. The problem is still there!

https://dutscher.github.io/stencilslots/

cheers

@dutscher
Copy link
Contributor Author

dutscher commented Mar 3, 2020

updated to 1.8.10 and no fix for this.
add enough

around the slot's fixxes is but its not the fine english way.

cheers

@Aswathprabhu
Copy link

Facing this in 1.17.3

@sajTempler
Copy link

possible workaround:
#2036 (comment)

@rwaskiewicz rwaskiewicz added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label Jul 17, 2023
@ionitron-bot ionitron-bot bot removed the triage label Jul 17, 2023
rwaskiewicz added a commit that referenced this issue Jul 28, 2023
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Sep 26, 2023
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Dec 6, 2023
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Dec 6, 2023
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Dec 7, 2023
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Dec 11, 2023
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Dec 15, 2023
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Dec 15, 2023
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 3, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 8, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
tanner-reits pushed a commit that referenced this issue Jan 9, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 9, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 17, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 17, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 17, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 22, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 24, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs pushed a commit that referenced this issue Jan 26, 2024
this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue
alicewriteswrongs added a commit that referenced this issue Jan 26, 2024
This adds a new transformer, `performAutomaticKeyInsertion`, which will
add `key` attributes into certain JSX nodes in the `render()` method of
a Stencil component. This will allow the Stencil runtime to make more
accurate decisions about when to create and destroy child nodes during
re-rendering, especially in circumstances where nodes are changing order
and so on.

There are some limitations on when we can safely insert keys without
possibly introducing incorrect behavior. In particular, we don't want to
insert keys in the following situations:

- when a `render` method has more than one return statement
- when a `render` method has a conditional (ternary) expression in it
- when a JSX node is located inside of a JSX expression (i.e. within
  curly braces like `<div>{ ... }</div>`)

In each of these cases we don't have the static analysis chops to know
when a given JSX syntax tree node is supposed to correspond to the same
HTML element at runtime, whereas in the 'single return, JSX children
case' like:

```tsx
class Component {
  render () {
    return <div><img /></div>
  }
}
```

We know without a doubt in this case that the `div` and `img` JSX nodes
are always supposed to correspond to the same HTML elements at runtime,
so it's no problem to add keys to them.

The key insertion transformer also does not transform JSX nodes which
are not part of Stencil components, so if a project had, for some
reason, a React component or something in it we will leave it alone.

fixes #1968
fixes #5263
STENCIL-893
github-merge-queue bot pushed a commit that referenced this issue Jan 26, 2024
…5143)

* test(slot): add karma test for #1968

this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue

* feat(runtime): automatically insert `key` attrs during compilation

This adds a new transformer, `performAutomaticKeyInsertion`, which will
add `key` attributes into certain JSX nodes in the `render()` method of
a Stencil component. This will allow the Stencil runtime to make more
accurate decisions about when to create and destroy child nodes during
re-rendering, especially in circumstances where nodes are changing order
and so on.

There are some limitations on when we can safely insert keys without
possibly introducing incorrect behavior. In particular, we don't want to
insert keys in the following situations:

- when a `render` method has more than one return statement
- when a `render` method has a conditional (ternary) expression in it
- when a JSX node is located inside of a JSX expression (i.e. within
  curly braces like `<div>{ ... }</div>`)

In each of these cases we don't have the static analysis chops to know
when a given JSX syntax tree node is supposed to correspond to the same
HTML element at runtime, whereas in the 'single return, JSX children
case' like:

```tsx
class Component {
  render () {
    return <div><img /></div>
  }
}
```

We know without a doubt in this case that the `div` and `img` JSX nodes
are always supposed to correspond to the same HTML elements at runtime,
so it's no problem to add keys to them.

The key insertion transformer also does not transform JSX nodes which
are not part of Stencil components, so if a project had, for some
reason, a React component or something in it we will leave it alone.

fixes #1968
fixes #5263
STENCIL-893

---------

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>
@rwaskiewicz
Copy link
Member

The fix for this issue has gone out in today's v4.12.0 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil slot-related
Projects
None yet
5 participants