bug: v4 Scoped Component Issues

[yigityuce]

Prerequisites

• I have read the Contributing Guidelines.
• I agree to follow the Code of Conduct.
• I have searched for existing issues that already report this problem, without success.

Stencil Version

4.12.1

Current Behavior

Regarding the discussion in here we started to test out the "scoped" component approach. Please take a look at our checklist below and the status.

Terms:

• "with slot fixes": experimentalSlotFixes and scopedSlotTextContentFix configs are applied
• "without slot fixes": experimentalSlotFixes and scopedSlotTextContentFix configs are NOT applied

Status:

Test Case	Description	with slot fixes	without slot fixes	PR
✅ Dynamic Tag	We need to have a dynamically updatable slot parent tag name to pass the accessibility audits	OK	OK	-
✅ Dynamic Sibling Children	We need to have dynamically re-orderable or conditionally rendered default slot sibling elements	OK	OK	-
Nested Relocated & Dynamic Sibling Children	We need to have dynamically re-orderable children and also these children elements can be passed through several components via using default slot	FAIL	FAIL	#5403
Conditional Rendering (named slot)	We need to have conditionally rendered named slot element(s)	FAIL	OK	#5365
Conditional Rendering (default slot)	We need to have conditionally rendered default slot element(s)	PARTIALLY1	PARTIALLY1	#5365
Slot fallback	We need to have stable slot fallback	FAIL1	FAIL1	-
Slot ref handling	We need to be able to handle the ref attribute for slots	FAIL	FAIL	#5337
Update "textContent"	We need to be able to update the default slot text by using component's textContent attribute	FAIL	FAIL	#5354
Update "innerText"	We need to be able to update the default slot text by using component's innerText attribute	FAIL	FAIL	-

¹: slot contents are hidden but slot fallback content is not visible

Expected Behavior

all the cases should work

System Info

System: node 16.18.0
    Platform: darwin (23.1.0)
   CPU Model: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz (12 cpus)
    Compiler: /Users/yigit.yuce/Documents/Projects/personal/yigityuce.github/stencil-v4-scoped-issues/node_modules/@stencil/core/compiler/stencil.js
       Build: 1707148558
     Stencil: 4.12.1 🏸
  TypeScript: 5.3.3
      Rollup: 2.56.3
      Parse5: 7.1.2
      jQuery: 4.0.0-pre
      Terser: 5.27.0

Steps to Reproduce

explained in the current behavior section and also the main-app component in the repo is self-explanatory

Code Reproduction URL

https://github.com/yigityuce/stencil-v4-scoped-issues

Additional Information

No response

[yigityuce]

added a new failing case to the bugs table:

Test Case	Description	with slot fixes	without slot fixes	PR
Nested Relocated & Dynamic Sibling Children	We need to have dynamically re-orderable children and also these children elements can be passed through several components via using default slot	FAIL	FAIL	-

a failing example is provided in the reproduction repo with this commit. As you can see in the "Nested Relocated & Dynamic Sibling Children Example":

1. On the initial render when the default slot doesn't have a wrapper, the "item-0" is always rendered before the last item which should have been rendered as the first (fixed by: fix(renderer): add check for already relocated nodes to avoid reordering)
2. when you hit the "Randomize" button, the new default slot items are added to the first "dummy-wrapper" component, but they should have been added to the 2 levels nested and another "dummy-wrapper" component which is located in a completely different place. (fixed by: fix(renderer): add hostname to host slot node finder method)

[yigityuce]

@rwaskiewicz is there any update on the task and the PRs? It's been almost more than 3 weeks and we really need these patches

[itsjustaplant]

Any update on this?

[ClaudiaMarchPiris]

I am also interested in this update!

[rwaskiewicz]

Hey folks, a couple of the PRs are still scheduled for the team to take a look at this sprint. Please note that we have other initiatives going on at the moment, and appreciate your patience here while we get around to them.

[rwaskiewicz]

Hey @yigityuce - can you please provide a minimal reproduction case for each of the issues laid out here? Right now, the team would like a concrete way to see what's failing currently and how each PR solves a specific problem.

To move forward with the linked PRs, can you do the following for each PR:

1. Spin up a new Stencil component starter project with npm init stencil@latest component
2. Update the component library to showcase the error

From there, we can better understand the general problem as well as build Stencil for each PR and evaluate the fix. Ultimately, we'd likely use this minimal reproduction cases as automated tests in our codebase.

We'd greatly appreciate it if there was a separate project per issue. That'll help us immensely when it comes to evaluating reproduction cases.

Let me know if you have any questions, thanks!

[yigityuce]

Hey @rwaskiewicz nice to hear back from you!

Here are the separated repositories for each issue fixed by each PR. I hope this helps you. Please do not hesitate to contact me for any further questions or requests. I will try to help you as much as possible.

Fix PR	Reproduction Repo	Notes
#5337	https://github.com/yigityuce/stencil-v4-sc-slot-ref/commits/master/	slot refs are not getting called
#5365	https://github.com/yigityuce/stencil-v4-sc-conditional-rendering/commits/master/	hidden attr is not set properly
#5403	https://github.com/yigityuce/stencil-v4-sc-deep-nested-slot-reordering/commits/master/	item-0 should have been placed as a first item but it is always reordered to (n-1)th place
#5403	https://github.com/yigityuce/stencil-v4-sc-deep-nested-dynamic-slot-relocation/commits/master/	dynamically regenerated items get relocated to the wrong place
#5354	https://github.com/yigityuce/stencil-v4-sc-default-slot-textcontent-update/commits/master/	when you set anything to the component's textContent attribute, all the children elements get removed and new text is inserted. This might NOT be a bug and might be done intentionally. However, it would be great to have a flag to change the behavior to only update the default slot's content by mutating the component's textContent attribute
#5496	https://github.com/yigityuce/stencil-v4-sc-slot-fallback-with-textnode/commits/master/	When a text node is provided to the scoped component as a slot, the slot fallback content does not become hidden.

[christian-bromann]

@yigityuce in regards to dynamically regenerated items get relocated to the wrong place: in the reproduction case you provide you are using an event handler that updates the DOM directly:

<button
  onClick={() => {
    const items = Array.from(this.nestedTestExampleRef?.querySelectorAll('.default-slot-item') || []);
    for (let i = 0; i < items.length; i++) {
      items[i].remove();
    }
    for (const item of this.createTestElements(getRandomInt(1, 20))) {
      this.nestedTestExampleRef?.appendChild(item);
    }
  }}
>
Randomize Item Count
</button>

I believe this kind of DOM manipulation is not allowed within JSX and languages that maintain a virtual DOM as you confuse the components internal state about its virtual DOM. Instead, I recommend working with the state of the application to have the component re-render itself correctly. Here is a working example of the same use case:

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.css',
  shadow: false,
  scoped: true,
})
export class MyComponent implements ComponentInterface {
  @State() dynamicTestItems: HTMLElement[] = [];
  @State() itemCount = DEFAULT_ITEM_COUNT;

  componentWillLoad(): void | Promise<void> {
    this.dynamicTestItems = this.createTestVNodes();
  }

  private createTestVNodes = (shuffle = false): HTMLElement[] => {
    const items = Array.from(new Array(this.itemCount)).map((_, i) => <span class="default-slot-item">{`item-${i}`}</span>);
    return shuffle ? items.sort(() => Math.random() - 0.5) : items;
  };

  render() {
    console.log();

    return (
      <Host>
        <my-nested-component>
          <span slot="header">Header Text</span>
          {this.dynamicTestItems}
        </my-nested-component>

        <button
          onClick={() => {
            this.itemCount = getRandomInt(1, 20);
            this.dynamicTestItems = this.createTestVNodes();
          }}
        >
          Randomize Item Count
        </button>
      </Host>
    );
  }
}

[yigityuce]

Hey @christian-bromann yes for the fwks that use VDom internally you are right, but how about the VanillaJS?

For instance, we are using storybook-html to showcase the components that we develop, and we do the DOM operations by calling the DOM API's as in the example. Also at the end of the day, all the frameworks will call these APIs (remove, insert, append, etc) to reflect the VDOM to the real DOM. Am I missing sth?

[christian-bromann]

Am I missing sth?

Stencil uses a virtual DOM implementation (based on Preact) to calculate DOM changes is memory first before using DOM APIs which are expensive operations. The minimal additional Stencil code that ships with your component contains this. So even if you use your component in a VanillaJS environment, internally your component uses a virtual DOM for updates. Now, the reason for the virtual DOM is that we want to be efficient on the changes made to the component and don't cause any unnecessary re-rendering. When it does re-render elements it uses the same WebAPIs to modify the DOM but it only updates the elements that require changes based on the state in the virtual DOM.

Now, if you manually change the elements in your component, you create a divergence from what the component has in memory and what the reality actually looks like which creates unforeseeable side effects and therefor is discouraged.

[yigityuce]

yeah yeah I know, but what I am trying to say is; think that this operation is not doing within the Stencil component. This is just for an example purpose. In real application user will do the same from the app level.

I just created a branch from that reproduction repo and here is the commit to see the diff. Basically I moved the DOM mutation logic to outside of the stencil component and moved it to the index.html and the result is same

I hope it would be useful to better understand the use case and sorry for creating the confusion by not providing the best example

[christian-bromann]

think that this operation is not doing within the Stencil component. This is just for an example purpose. In real application user will do the same from the app level.

Users should not manipulate the DOM individual components directly either. There is no point within the Stencil docs where this is a recommend approach. Mind providing more context when someone would be interested doing this? A component should provide enough parameter or event handlers to have it handle everything accordingly. Manipulating DOM nodes within arbitrary components seem like an antipattern to me.

Thank you for providing an update to the example. It clears up a lot of things. What I am saying above definitely doesn't count for the light DOM because the slot content is not handled by the component itself. This is indeed a bug and as you pointed out it could be fixed with #5403.

[yigityuce]

Users should not manipulate the DOM individual components directly either.

Yeap they shouldn't but they do 😩 also at the end of the day eventually fwks will do it to reflect the VDOM to the light dom. (react implementation)

Mind providing more context when someone would be interested doing this

I'm putting an example mostly in pseudo code, but I think this will give you the idea about one of the use case in our kit. For example in that example, the dropdowns' content will be rendered dynamically with the BE data regarding the selection of previous dropdown.

Side note: yeah I know we can retrieve the data as prop and render it by using another prop such as "renderer" function callback. But we provide an UI library that user can combine different elements (we need to define complex data structure to handle every use case) and also another question is what would be the renderer function return type? React's ReactNode? Angular's ComponentRef? Vue's Component?

// filter.tsx in a react project

const FilterPanel: FC = () => {
  const [brand, setBrand] = useState<string>();
  const [model, setModel] = useState<string>();
  const [fuelType, setFuelType] = useState<string>();

  const brands = useFetcher(() => fetch('get-brands'), []);
  const models = useFetcher(() => fetch('get-models'), [rand]);
  const fuelTypes = useFetcher(() => fetch('get-brands'), [brand, model]);

  return (
    <div>
       <my-dropdown value={brand} onValueChange={({ detail }) => setBrand(detail)}>
         {brands.map((item) => <my-list-item value={item.id}>{item.name}</my-list-item>)}
       </my-dropdown>

       <my-dropdown value={model} onValueChange={({ detail }) => setModel(detail)}>
         {models.map((item) => <my-list-item value={item.id}>{item.name}</my-list-item>)}
       </my-dropdown>
       
       <my-dropdown value={fuelType} onValueChange={({ detail }) => setFuelType(detail)}>
         {fuelTypes.map((item) => <my-list-item value={item.id}>{item.name}</my-list-item>)}
       </my-dropdown>
    </div>
  )
}

// component lib file
@Component({  tag: 'my-dropdown' })
export class MyListItem {

  render() {
    return (
       <Host>
         <my-input>
            <div slot="prefix"><slot name="icon" /></div>
            <my-icon slot="suffix" name="caret_down" />
         </my-input>
         <my-popover anchor="...">
           <my-list searchable dense divider>
              <slot />
           </my-list>
         </my-popover>
       </Host>
    );

  }
}

This is indeed a bug and as you pointed out it could be fixed with #5403.

I will fix the issue there and provide e2e test ASAP. So I assume, it will be merged when it is ready, right?

[christian-bromann]

I'm putting an example mostly in pseudo code, but I think this will give you the idea about one of the use case in our kit.

Thanks a lot, this clears things up. For some reason I was thinking that users try to manipulate the DOM within the custom component. You case nicely demonstrates that we are talking about the light DOM that gets slotted into it which is out if control of the component and totally can be changing at any time.

I will fix the issue there and provide e2e test ASAP. So I assume, it will be merged when it is ready, right?

There are no objections with the changes made in #5403. Once you can fix the issue and provide a test for it we will review as a team and eventually move forward merging it if no team member raises any concerns.

Thanks for all your work, this has been great!

[yigityuce]

updated the "Reproduction Repo & Fix PR" table comment with the new issue and its fix.

FYI: @christian-bromann @rwaskiewicz

Hey @rwaskiewicz nice to hear back from you!

Here are the separated repositories for each issue fixed by each PR. I hope this helps you. Please do not hesitate to contact me for any further questions or requests. I will try to help you as much as possible.

Fix PR Reproduction Repo Notes
#5337 https://github.com/yigityuce/stencil-v4-sc-slot-ref/commits/master/ slot refs are not getting called
#5365 https://github.com/yigityuce/stencil-v4-sc-conditional-rendering/commits/master/ hidden attr is not set properly
#5403 https://github.com/yigityuce/stencil-v4-sc-deep-nested-slot-reordering/commits/master/ item-0 should have been placed as a first item but it is always reordered to (n-1)th place
#5403 https://github.com/yigityuce/stencil-v4-sc-deep-nested-dynamic-slot-relocation/commits/master/ dynamically regenerated items get relocated to the wrong place
#5354 https://github.com/yigityuce/stencil-v4-sc-default-slot-textcontent-update/commits/master/  when you set anything to the component's textContent attribute, all the children elements get removed and new text is inserted. This might NOT be a bug and might be done intentionally. However, it would be great to have a flag to change the behavior to only update the default slot's content by mutating the component's textContent attribute

[yigityuce]

I believe all the issues are fixed and the PRs are merged, so it's time to close this issue 🕺 thanks everyone for their support and helping to move forward! see you in other issues 😇

special thanks to @dotNetkow and all the team members for their responsiveness and proactivity!

(@christian-bromann & @tanner-reits & @rwaskiewicz you guys rock! 🚀 )