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

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

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

alicewriteswrongs
Copy link
Contributor

@alicewriteswrongs alicewriteswrongs commented Dec 7, 2023

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

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

github-actions bot commented Dec 7, 2023

--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

Copy link
Contributor Author

@alicewriteswrongs alicewriteswrongs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noting a few things

src/compiler/transformers/automatic-key-insertion/index.ts Outdated Show resolved Hide resolved
src/compiler/transformers/automatic-key-insertion/index.ts Outdated Show resolved Hide resolved
@@ -337,7 +337,7 @@ describe('render-vdom', () => {
vdomXlink: false,
vdomClass: false,
vdomStyle: false,
vdomKey: false,
vdomKey: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</div>
);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ditto for the other stuff in this directory)

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

@alicewriteswrongs
Copy link
Contributor Author

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

@alicewriteswrongs alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from df7b575 to 3be7b5b Compare December 11, 2023 15:47
alicewriteswrongs added a commit that referenced this pull request Dec 13, 2023
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.
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2023
…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.
@alicewriteswrongs 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 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 alicewriteswrongs marked this pull request as ready for review January 8, 2024 20:55
@alicewriteswrongs alicewriteswrongs requested a review from a team as a code owner January 8, 2024 20:55
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@alicewriteswrongs alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from 051d38e to cea23c4 Compare January 9, 2024 19:33
src/compiler/transformers/automatic-key-insertion/index.ts Outdated Show resolved Hide resolved
Comment on lines 906 to 915
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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@alicewriteswrongs alicewriteswrongs Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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:

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:

  1. there are two returns in a component's render method and
  2. 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.

@rwaskiewicz
Copy link
Member

Ahh - two more things:

  1. 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?
  2. Can we add some documentation around this behavior for the Stencil Site?

@alicewriteswrongs 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 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 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
Copy link
Contributor Author

@alicewriteswrongs alicewriteswrongs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just noting a few things

Comment on lines +203 to +237
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';
}
}
`,
);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +52 to +58
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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

@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!

Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good!

I have two asks before we can merge this:

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

Comment on lines 906 to 915
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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from e73e6bf to 7631132 Compare January 24, 2024 15:54
@alicewriteswrongs
Copy link
Contributor Author

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

@alicewriteswrongs
Copy link
Contributor Author

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

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

rwaskiewicz and others added 2 commits January 26, 2024 14:31
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
@alicewriteswrongs alicewriteswrongs force-pushed the ap/parent-with-child-slot-rerender-bug branch from 1b9e0da to 1df736e Compare January 26, 2024 19:43
@alicewriteswrongs 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 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
alicewriteswrongs added a commit to ionic-team/stencil-site that referenced this pull request Jan 26, 2024
alicewriteswrongs added a commit that referenced this pull request Mar 26, 2024
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 added a commit that referenced this pull request Mar 26, 2024
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 added a commit that referenced this pull request Apr 3, 2024
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 added a commit that referenced this pull request Apr 5, 2024
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
github-merge-queue bot pushed a commit that referenced this pull request Apr 5, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants