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

alicewriteswrongs · 2023-12-07T21:36:27Z

Putting up the code for this now so we can start looking at it!

What is the current behavior?

We have a number of issues in the backlog relating to shortcomings in how children are reconciled across virtual DOM re-renders at present. For example, #1968 is caused by an interaction between a failure to correctly recognize the same child across a re-render and another issue with slot content relocation in a scoped context.

This implements an approach to addressing a subset of these issues by automatically inserting key attributes at compile-time, making it easier at run-time for the virtual DOM to determine when two children of a virtual DOM node should be matched up with each other.

What is the new behavior?

Now as part of transpilation Stencil will insert randomly-generated key attributes into JSX syntax tree nodes. This allows the more-accurate key-based child reconciliation algorithm to be used almost all the time, fixing some edge cases where previously the Stencil runtime would accidentally misplace a child, failing to, for instance, correctly match up an old and new vdom node when nodes were reordered.

Does this introduce a breaking change?

I don't believe it does!

Yes
No

Testing

I have added some tests which exercise the change and also ensured that all of our existing tests are passing without issue.

I also built and ran this in Framework, where all of the unit tests are passing and some smoke testing of components (the datetime in particular) doesn't reveal any potential problems.

But! For testing this I think it would be a good idea to check out the reproduction case from #1968. Then:

confirm you can reproduce the issue with @stencil/core@latest
checkout this branch
npm run build && npm pack
install the tarball in the reproduction case
confirm that the issue is fixed

github-actions · 2023-12-07T21:38:12Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1207 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/mock-doc/serialize-node.ts	36
src/dev-server/server-process.ts	32
src/compiler/prerender/prerender-main.ts	22
src/testing/puppeteer/puppeteer-element.ts	22
src/runtime/client-hydrate.ts	20
src/screenshot/connector-base.ts	19
src/runtime/vdom/vdom-render.ts	17
src/compiler/config/test/validate-paths.spec.ts	16
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/compiler/transpile/transpile-module.ts	14
src/sys/node/node-sys.ts	14
src/compiler/prerender/prerender-queue.ts	13
src/compiler/sys/in-memory-fs.ts	13
src/runtime/connected-callback.ts	13
src/runtime/set-value.ts	13
src/compiler/output-targets/output-www.ts	12
src/compiler/transformers/test/parse-vdom.spec.ts	12

Our most common errors

Typescript Error Code	Count
TS2345	366
TS2322	362
TS18048	224
TS18047	99
TS2722	37
TS2532	26
TS2531	23
TS2454	14
TS2790	11
TS2352	10
TS2769	8
TS2538	8
TS2344	6
TS2416	6
TS2493	3
TS18046	2
TS2684	1
TS2430	1

Unused exports report

There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports

File	Line	Identifier
src/runtime/bootstrap-lazy.ts	21	setNonce
src/screenshot/screenshot-fs.ts	18	readScreenshotData
src/testing/testing-utils.ts	198	withSilentWarn
src/utils/index.ts	145	CUSTOM
src/utils/index.ts	269	normalize
src/utils/index.ts	7	escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	115	Env
src/compiler/app-core/app-data.ts	117	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	61	satisfies
src/compiler/types/validate-primary-package-output-target.ts	61	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

alicewriteswrongs

noting a few things

src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts

src/compiler/transformers/automatic-key-insertion/index.ts

alicewriteswrongs · 2023-12-07T21:40:12Z

src/runtime/test/render-vdom.spec.tsx

@@ -337,7 +337,7 @@ describe('render-vdom', () => {
        vdomXlink: false,
        vdomClass: false,
        vdomStyle: false,
-        vdomKey: false,
+        vdomKey: true,


these tests for vdomKey just all changed because now every build has key attrs in it

alicewriteswrongs · 2023-12-07T21:40:49Z

test/karma/test-app/scoped-conditional/cmp.tsx

+      </div>
+    );
+  }
+}


this is from a commit created by @rwaskiewicz which reproduces #1968 as a karma test.

(ditto for the other stuff in this directory)

before adding the transformer these tests were failing, now they pass!

alicewriteswrongs · 2023-12-07T21:48:37Z

I just published @stencil/core@4.8.1-dev.1701985394.df7b575 from this branch, so that can be used to assess whether this addresses open issues

This makes it possible to set a key attribute on a nested Stencil component without messing up how hydration works. This fixes a bug which was noticed while working on #5143.

…5164) This makes it possible to set a key attribute on a nested Stencil component without messing up how hydration works. This fixes a bug which was noticed while working on #5143.

christian-bromann

LGTM 👍

src/compiler/transformers/automatic-key-insertion/utils.ts

src/compiler/transformers/automatic-key-insertion/index.ts

src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts

rwaskiewicz · 2024-01-09T20:11:30Z

src/runtime/test/render-vdom.spec.tsx

      expect(rect.getAttribute('transform')).toBe(null);

      root.isOpen = true;
      await waitForChanges();
+      rect = root.querySelector('rect');
      expect(rect.getAttribute('transform')).toBe('rotate(45 27 27)');

      root.isOpen = false;
      await waitForChanges();
+      rect = root.querySelector('rect');


For my understanding, why do we need to re-query for 'rect' now?

a small difference in how the elements get matched up means that where before a new rect element wouldn't be created on re-render it now will be, so the reference to the rect element created on the first render isn't to the updated element created on the second re-render

Does that apply to Stencil tests as well? Could that be considered breaking for folks that are writing tests similar to this?

It's possible if a stencil user is writing very detailed tests that assert that within a particular component a given HTML element is either destroyed and recreated or that it's conserved across re-renders they might need to rewrite some tests. I have tested in Framework and all the tests there pass with no issues, so that's a good sign at least. Possibly we want to alert users to this? I'm not sure I would really want to classify it as a breaking change though since I think that we are fully within our rights to 'claim' the specific behavior of how Stencil's vdom produces the actual DOM that the user declaratively specifies as an implementation detail, and not itself part of the public API for building Stencil components and so on. I also haven't been able to produce counter-examples for myself which would show different user-facing behavior with this change, only 'under the hood' differences, and I think it's likely that most users wouldn't need to edit their tests with this change

I'm not sure I agree with the premise - we may be within our right to claim behavior here, but I don't think the user would necessarily understand the distinction here. They'd just see broken tests that "worked before".

But! That all may be moot - I think in
applying this patch (revert-tests.patch, we can revert these changes safely with the latest round of commits and have the tests still pass 👍

rwaskiewicz · 2024-01-09T20:23:06Z

test/karma/test-app/slot-reorder/karma.spec.ts

@@ -72,7 +75,7 @@ describe('slot-reorder', () => {
      expect(r.children[0].hasAttribute('hidden')).toBe(false);
      expect(r.children[0].getAttribute('name')).toBe('slot-b');
      expect(r.children[1].textContent.trim()).toBe('fallback default');
-      expect(r.children[1].hasAttribute('hidden')).toBe(false);
+      expect(r.children[1].hasAttribute('hidden')).toBe(true);


I'm not sure I fully understand this - the implication of this code change to me is that the 'hidden' attribute's presence/value would be changing from today's existing behavior for scoped components - can you help me understand if this is the case / why we need to change this value?

I believe what's going on is that previously this element was being destroyed and re-created so that it didn't have the hidden attribute anymore, now it is preserved across re-renders so although the value of el.getAttribute('hidden') is I believe consistent and what it's supposed to be we see a difference in whether or not the attribute has been set on the element. Does that make sense?

I'm not sure, I'm still not totally clear as to how this is a result of automatic key insertion. Who/what was doing the recreation before, and who's doing it now? I suppose what I'm trying to understand here is any differences in our render cycle that might be a result of this change, and this question is just a small part of that.

I realized that this is another situation where I think we want to bail before adding keys to the JSX. This is the component that's being tested in this file:

stencil/test/karma/test-app/slot-reorder/cmp.tsx

Lines 1 to 40 in 148e8cd

import { Component, Prop, h } from '@stencil/core';

@Component({

tag: 'slot-reorder',

})

export class SlotReorder {

@Prop() reordered = false;

render() {

if (this.reordered) {

return (

<div class="reordered">

<slot name="slot-b">

<div>fallback slot-b</div>

</slot>

<slot>

<div>fallback default</div>

</slot>

<slot name="slot-a">

<div>fallback slot-a</div>

</slot>

</div>

);

}

return (

<div>

<slot>

<div>fallback default</div>

</slot>

<slot name="slot-a">

<div>fallback slot-a</div>

</slot>

<slot name="slot-b">

<div>fallback slot-b</div>

</slot>

</div>

);

}

}

In particular, note the render() method has two returns, each of which wrap some slot stuff with <div> tags, like so:

export class SlotReorder { @Prop() reordered = false; render() { if (this.reordered) { return ( <div class="reordered"> { ... } </div> ); } return ( <div> { ... } </div> ); } }

in this case we can't safely add key attrs to those divs, because doing so will cause the runtime to not match them up across renders. In particular, our implementation of isSameVnode assumes that if keys are set on two vnodes then those are authoritative:

stencil/src/runtime/vdom/vdom-render.ts

Lines 586 to 604 in 148e8cd

export const isSameVnode = (leftVNode: d.VNode, rightVNode: d.VNode, isInitialRender = false) => {

// compare if two vnode to see if they're "technically" the same

// need to have the same element tag, and same key to be the same

if (leftVNode.$tag$ === rightVNode.$tag$) {

if (BUILD.slotRelocation && leftVNode.$tag$ === 'slot') {

return leftVNode.$name$ === rightVNode.$name$;

}

// this will be set if JSX tags in the build have `key` attrs set on them

// we only want to check this if we're not on the first render since on

// first render `leftVNode.$key$` will always be `null`, so we can be led

// astray and, for instance, accidentally delete a DOM node that we want to

// keep around.

if (BUILD.vdomKey && !isInitialRender) {

return leftVNode.$key$ === rightVNode.$key$;

}

return true;

}

return false;

};

Note in particular that after we compare the tags on the nodes we then do this check:

if (BUILD.vdomKey && !isInitialRender) { return leftVNode.$key$ === rightVNode.$key$; }

The transformed version of the component in question here would look something like this, with keys inserted:

export class SlotReorder { @Prop() reordered = false; render() { if (this.reordered) { return ( <div class="reordered" key="asdfasdf1234"> { ... } </div> ); } return ( <div key="hjklhjkl5678"> { ... } </div> ); } }

because the keys set on those syntax nodes are not equal, when the vdom assesses whether or not these nodes are equal it will see that they have key attrs and, in that case, it will say "ok, are these equal? no? then these are not the same vnode". This will cause it to then re-render the whole thing, leading to some undesirable behavior where the <div> actually should be kept around between re-renders so that its children can be diffed as well, rather than always being re-rendered on each render cycle.

I thought about this a bit, and I think that there's just not really a safe way that we can add keys in a case like this where there are multiple returns from the component's render() method. Or, at the least, I don't think we could safely do so without adding some more fancy static analysis that I dont think we want to take on. Like adding keys would be safe here I think:

export class ImageOrText { @Prop() showImage = false; render() { if (this.showImage) { return ( <img src="..." /> ); } return ( <span>text instead!</span> ); } }

because the two tags are different the vdom would already have to re-render the elements when this.showImage changes, so we wouldn't run into problems adding keys to the img and span tags in the JSX. But! I don't think we probably want to bite off trying to figure out when it's safe to do this. So instead I think we need to not add keys when:

there are two returns in a component's render method and

a component's render method returns a ternary

Does that all make sense? I've added some further constraints to when we transform JSX nodes and it makes this test in pass without the changes I have in this file right now.

src/compiler/transformers/automatic-key-insertion/utils.ts

rwaskiewicz · 2024-01-09T20:29:01Z

Ahh - two more things:

I think we should mark this in a minor release of Stencil/mark it as a feature. Can we update the PR name/commit messages?
Can we add some documentation around this behavior for the Stencil Site?

alicewriteswrongs

just noting a few things

alicewriteswrongs · 2024-01-17T19:38:01Z

src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts

+  it('should not add a static key to dynamic elements', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValueOnce('once-key');
+    const t = transpile(`
+     @Component({tag: 'cmp-a'})
+     export class CmpA {
+        render() {
+          return (
+            <div>
+              {this.todos.map((todo) => (
+                <div>{ todo }</div>
+              ))}
+            </div>
+          )
+      }
+    }`);
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h(
+      'div',
+      { key: 'once-key' },
+      this.todos.map((todo) => h('div', null, todo)),
+    );
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });


this is a test to ensure that we don't add static keys to dynamic generated elements. I did a bit of a conservative cut-off for these - we don't attempt to transform any JSX nodes which are found within a JSX expression, so with (contrived) code like

@Component({ tag: 'cmp-a' }) class CmpA { render() { return <div>{ (() => <div>inner</div>)() }</div> } }

we won't transform the 'inner' div in the IIFE because, since it will be generated at runtime, we could run into a lot of cases where we can't really know too much about what's going on there and when it would not be safe to add keys.

alicewriteswrongs · 2024-01-17T19:40:29Z

src/compiler/transformers/automatic-key-insertion/index.ts

+      const tagName = getComponentTagName(node.members.filter(isStaticGetter));
+      if (tagName != null) {
+        // we've got a class node with an `is` property, which tells us that
+        // the class we're dealing with is a Stencil component which has
+        // already been through the `convertDecoratorsToStatic` transformer.
+        return ts.visitEachChild(node, findRenderMethodVisitor, transformCtx);
+      }


this is the same approach we use in parseStaticComponentMeta to figure out whether a class is a Stencil component or not, see here:

stencil/src/compiler/transformers/static-to-meta/component.ts

Lines 49 to 53 in d6e9eac

const staticMembers = cmpNode.members.filter(isStaticGetter);

const tagName = getComponentTagName(staticMembers);

if (tagName == null) {

return cmpNode;

}

this ensures that we only continue with transforming the syntax tree if we're dealing with a Stencil component, so if you have for instance a class-based React component in your source code it should not be transformed

src/compiler/transformers/automatic-key-insertion/index.ts

alicewriteswrongs · 2024-01-17T19:44:08Z

@rwaskiewicz I believe this is ready for another look! I've addressed your comments and added a few more tests to cover some more edge cases, lmk what you think!

rwaskiewicz

Overall looks good!

I have two asks before we can merge this:

I had one change I'd like to make to the code in its current state (comment made as a part of this review)
Did we ever push the PR up containing docs around this functionality?

rwaskiewicz · 2024-01-24T14:59:42Z

src/runtime/test/render-vdom.spec.tsx

      expect(rect.getAttribute('transform')).toBe(null);

      root.isOpen = true;
      await waitForChanges();
+      rect = root.querySelector('rect');
      expect(rect.getAttribute('transform')).toBe('rotate(45 27 27)');

      root.isOpen = false;
      await waitForChanges();
+      rect = root.querySelector('rect');


I'm not sure I agree with the premise - we may be within our right to claim behavior here, but I don't think the user would necessarily understand the distinction here. They'd just see broken tests that "worked before".

But! That all may be moot - I think in
applying this patch (revert-tests.patch, we can revert these changes safely with the latest round of commits and have the tests still pass 👍

alicewriteswrongs · 2024-01-24T15:54:44Z

@rwaskiewicz I removed those changes, thanks for checking if they were still necessary!

alicewriteswrongs · 2024-01-24T16:11:18Z

also I opened a documentation here: ionic-team/stencil-site#1330

christian-bromann

Struggled a bit to reproduce the problem but with the help of @alicewriteswrongs reproducible example I was able that the issue is resolved with this patch 👍

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

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

This functionality was added to Stencil in ionic-team/stencil#5143

This expands the scope of the automatic key insertion feature that was added in #5143. Previously the feature stopped at the border of any `JsxExpression` syntax node (a bit of arbitrary JavaScript wrapped in curly braces and then inserted into JSX) so that if you were doing something like: ```tsx <div>{ someCondition && <span>when condition is true</span>}</div> ``` the compiler would insert a key into the `<div>` syntax tree node but _not_ the node for the `<span>`. We did this to just be a bit cautious with the feature because there are a lot of different things that could be going on within a `JsxExpression` node, and some things which could be written within curly braces which would not be safe to transform, such as the following: ```tsx <div>{ xs.map(x => <span>{ x }</span>) }</div> ``` We wouldn't want to insert a key into the `<span>` syntax tree node because that would result in the dynamically generated `<span>` tags always having the same key attributes. That said, there are some situations where it is safe to insert a key, such as the `condition || <some-tag />` case shown above. This change adds support for inserting keys in those situations. STENCIL-1120

…5594) This expands the scope of the automatic key insertion feature that was added in #5143. Previously the feature stopped at the border of any `JsxExpression` syntax node (a bit of arbitrary JavaScript wrapped in curly braces and then inserted into JSX) so that if you were doing something like: ```tsx <div>{ someCondition && <span>when condition is true</span>}</div> ``` the compiler would insert a key into the `<div>` syntax tree node but _not_ the node for the `<span>`. We did this to just be a bit cautious with the feature because there are a lot of different things that could be going on within a `JsxExpression` node, and some things which could be written within curly braces which would not be safe to transform, such as the following: ```tsx <div>{ xs.map(x => <span>{ x }</span>) }</div> ``` We wouldn't want to insert a key into the `<span>` syntax tree node because that would result in the dynamically generated `<span>` tags always having the same key attributes. That said, there are some situations where it is safe to insert a key, such as the `condition || <some-tag />` case shown above. This change adds support for inserting keys in those situations. STENCIL-1120

alicewriteswrongs changed the title ~~Ap/parent with child slot rerender bug~~ fix(runtime): address child reconciliation issues with automatic keys Dec 7, 2023

alicewriteswrongs commented Dec 7, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from df7b575 to 3be7b5b Compare December 11, 2023 15:47

alicewriteswrongs mentioned this pull request Dec 12, 2023

fix(runtime): allow setting key attr on nested Stencil components #5164

Merged

2 tasks

aniederer-chatham mentioned this pull request Dec 12, 2023

bug: broken rendering of slot elements on component rerender #5136

Open

3 tasks

alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch 2 times, most recently from ed4c36a to 9979366 Compare December 15, 2023 17:10

alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch 2 times, most recently from c96f92b to a690a63 Compare January 8, 2024 20:12

alicewriteswrongs marked this pull request as ready for review January 8, 2024 20:55

alicewriteswrongs requested a review from a team as a code owner January 8, 2024 20:55

alicewriteswrongs requested review from christian-bromann and rwaskiewicz January 8, 2024 20:55

christian-bromann approved these changes Jan 8, 2024

View reviewed changes

alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from 051d38e to cea23c4 Compare January 9, 2024 19:33

rwaskiewicz suggested changes Jan 9, 2024

View reviewed changes

tanner-reits linked an issue Jan 17, 2024 that may be closed by this pull request

bug: Conditional slot rendering removes always the last slot element, using key is a workaround #5263

Closed

3 tasks

alicewriteswrongs changed the title ~~fix(runtime): address child reconciliation issues with automatic keys~~ feat(runtime): add automatic insertion of key attrs during compilation Jan 17, 2024

alicewriteswrongs changed the title ~~feat(runtime): add automatic insertion of key attrs during compilation~~ feat(runtime): automatically insert key attrs during compilation Jan 17, 2024

alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch 2 times, most recently from 565798a to d6e9eac Compare January 17, 2024 19:31

alicewriteswrongs commented Jan 17, 2024

View reviewed changes

alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from d6e9eac to 53a92bc Compare January 17, 2024 19:43

alicewriteswrongs requested a review from rwaskiewicz January 17, 2024 19:44

alicewriteswrongs requested a review from rwaskiewicz January 22, 2024 20:12

rwaskiewicz suggested changes Jan 24, 2024

View reviewed changes

alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from e73e6bf to 7631132 Compare January 24, 2024 15:54

alicewriteswrongs requested a review from rwaskiewicz January 24, 2024 15:54

alicewriteswrongs mentioned this pull request Jan 24, 2024

docs(jsx): add explanation of automatic key insertion ionic-team/stencil-site#1330

Merged

rwaskiewicz approved these changes Jan 25, 2024

View reviewed changes

christian-bromann approved these changes Jan 26, 2024

View reviewed changes

rwaskiewicz and others added 2 commits January 26, 2024 14:31

test(slot): add karma test for #1968

349df98

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

alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from 1b9e0da to 1df736e Compare January 26, 2024 19:43

alicewriteswrongs enabled auto-merge January 26, 2024 19:43

alicewriteswrongs disabled auto-merge January 26, 2024 19:44

alicewriteswrongs enabled auto-merge January 26, 2024 19:44

alicewriteswrongs added this pull request to the merge queue Jan 26, 2024

Merged via the queue into main with commit 9c47438 Jan 26, 2024
120 checks passed

alicewriteswrongs deleted the ap/parent-with-child-slot-rerender-bug branch January 26, 2024 20:01

alicewriteswrongs added a commit to ionic-team/stencil-site that referenced this pull request Jan 26, 2024

docs(jsx): add explanation of automatic key insertion

9525183

This functionality was added to Stencil in ionic-team/stencil#5143

alicewriteswrongs added a commit to ionic-team/stencil-site that referenced this pull request Jan 26, 2024

docs(jsx): add explanation of automatic key insertion

30d26b1

This functionality was added to Stencil in ionic-team/stencil#5143

This was referenced Jan 29, 2024

Parent with Child Slot rerender bug #1968

Closed

bug: Conditional slot rendering removes always the last slot element, using key is a workaround #5263

Closed

michaelworm mentioned this pull request Jan 30, 2024

[BUG] - Flyout footer slot stays visible porsche-design-system/porsche-design-system#2955

Closed

alicewriteswrongs mentioned this pull request Mar 26, 2024

feat(compiler): perform automatic key insertion in more situations #5594

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alicewriteswrongs commented Dec 7, 2023 •

edited

github-actions bot commented Dec 7, 2023 •

edited

alicewriteswrongs left a comment

alicewriteswrongs Dec 7, 2023

alicewriteswrongs Dec 7, 2023

alicewriteswrongs Dec 7, 2023

alicewriteswrongs commented Dec 7, 2023

christian-bromann left a comment

rwaskiewicz Jan 9, 2024

alicewriteswrongs Jan 9, 2024

rwaskiewicz Jan 19, 2024

alicewriteswrongs Jan 19, 2024

rwaskiewicz Jan 24, 2024

rwaskiewicz Jan 9, 2024

alicewriteswrongs Jan 17, 2024

rwaskiewicz Jan 19, 2024

alicewriteswrongs Jan 22, 2024 •

edited

rwaskiewicz commented Jan 9, 2024

alicewriteswrongs left a comment

alicewriteswrongs Jan 17, 2024

alicewriteswrongs Jan 17, 2024

alicewriteswrongs commented Jan 17, 2024

rwaskiewicz left a comment

rwaskiewicz Jan 24, 2024

alicewriteswrongs commented Jan 24, 2024

alicewriteswrongs commented Jan 24, 2024

christian-bromann left a comment

	import { Component, Prop, h } from '@stencil/core';

	@Component({
	tag: 'slot-reorder',
	})
	export class SlotReorder {
	@Prop() reordered = false;

	render() {
	if (this.reordered) {
	return (
	<div class="reordered">
	<slot name="slot-b">
	<div>fallback slot-b</div>
	</slot>
	<slot>
	<div>fallback default</div>
	</slot>
	<slot name="slot-a">
	<div>fallback slot-a</div>
	</slot>
	</div>
	);
	}

	return (
	<div>
	<slot>
	<div>fallback default</div>
	</slot>
	<slot name="slot-a">
	<div>fallback slot-a</div>
	</slot>
	<slot name="slot-b">
	<div>fallback slot-b</div>
	</slot>
	</div>
	);
	}
	}

	export const isSameVnode = (leftVNode: d.VNode, rightVNode: d.VNode, isInitialRender = false) => {
	// compare if two vnode to see if they're "technically" the same
	// need to have the same element tag, and same key to be the same
	if (leftVNode.$tag$ === rightVNode.$tag$) {
	if (BUILD.slotRelocation && leftVNode.$tag$ === 'slot') {
	return leftVNode.$name$ === rightVNode.$name$;
	}
	// this will be set if JSX tags in the build have `key` attrs set on them
	// we only want to check this if we're not on the first render since on
	// first render `leftVNode.$key$` will always be `null`, so we can be led
	// astray and, for instance, accidentally delete a DOM node that we want to
	// keep around.
	if (BUILD.vdomKey && !isInitialRender) {
	return leftVNode.$key$ === rightVNode.$key$;
	}
	return true;
	}
	return false;
	};

	const staticMembers = cmpNode.members.filter(isStaticGetter);
	const tagName = getComponentTagName(staticMembers);
	if (tagName == null) {
	return cmpNode;
	}

feat(runtime): automatically insert key attrs during compilation #5143

feat(runtime): automatically insert key attrs during compilation #5143

Conversation

alicewriteswrongs commented Dec 7, 2023 • edited

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

github-actions bot commented Dec 7, 2023 • edited

--strictNullChecks error report

reports and statistics

Unused exports report

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Dec 7, 2023

christian-bromann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs Jan 22, 2024 • edited

Choose a reason for hiding this comment

rwaskiewicz commented Jan 9, 2024

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Jan 17, 2024

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Jan 24, 2024

alicewriteswrongs commented Jan 24, 2024

christian-bromann left a comment

Choose a reason for hiding this comment

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

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

alicewriteswrongs commented Dec 7, 2023 •

edited

github-actions bot commented Dec 7, 2023 •

edited

`--strictNullChecks` error report

alicewriteswrongs Jan 22, 2024 •

edited