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

docs(jsx): add explanation of automatic key insertion #1330

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

alicewriteswrongs
Copy link
Contributor

Add some documentation to our page on JSX which explains the changes made in ionic-team/stencil#5143

@alicewriteswrongs alicewriteswrongs requested a review from a team as a code owner January 24, 2024 16:10
@alicewriteswrongs alicewriteswrongs requested review from christian-bromann and rwaskiewicz and removed request for a team January 24, 2024 16:10
Copy link

vercel bot commented Jan 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
stencil-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 26, 2024 8:36pm

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.

When reading the new section I asked myself how the given example is different from the one just above it:

render() {
  return (
    <div>
      {this.todos.map((todo) => (
        <div key={todo.uid}>
          <div>{todo.taskName}</div>
        </div>
      ))}
    </div>
  )
}

Is this example outdated now since Stencil adds keys automatically?

@alicewriteswrongs
Copy link
Contributor Author

alicewriteswrongs commented Jan 24, 2024

hmm probably I need to explain better, I'll re-word a bit.

In short it's not outdated, because we decided in the implementation of ionic-team/stencil#5143 not to insert keys into JSX nodes which are within curly braces (a JSX expression) because there are just so many possibly things that could be going on there that it's very hard to know when it would be safe to add keys to elements without breaking the user's expectation for how their component should render.

An example would be a usage of Array.prototype.map like in that example. If the user had a component with something like:

render() {
  return (
    <div>
      {this.todos.map((todo) => (
        <div>
          <div>{todo.taskName}</div>
        </div>
      ))}
    </div>
  )
}

and we inserted keys automatically the transformed code would look something like this:

render() {
  return (
    <div>
      {this.todos.map((todo) => (
        <div key="a-key">
          <div>{todo.taskName}</div>
        </div>
      ))}
    </div>
  )
}

this is because the transformation is done at compilation-time and on the basis of the syntax tree nodes that the transformer finds, not at runtime based on the actual data found in this.todos. This would create a problem where the keys would make all the children look identical. So for that reason we need to not insert keys in three situations:

  1. within a JSX expression (i.e. the curly braces in <div>{ foo }</div>
  2. when a render method has more than one return
  3. when the first child of the return keyword in a render method is a conditional expression (aka ternary)

the 2nd and 3rd are necessary for us to avoid because in each we can't assume that the user's intent is for the two (or more) different branches in 'return behavior' to be matched up, and inserting keys in that situation would change the rendering behavior for a lot of cases.

Anywhoo - hopefully that helps explain a bit of how this works, I'll update to clarify more what is meant by the transformation not happening within a JSX expression

@christian-bromann
Copy link
Member

Thanks @alicewriteswrongs , that clears things up!

@alicewriteswrongs
Copy link
Contributor Author

@christian-bromann I updated this a bit, lmk what you think!

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 👍

Copy link
Contributor

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

LGTM - one minor suggestion that's non-blocking

Comment on lines 264 to 266
{this.disabled && (
<div />
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could strengthen this section a little bit by making it clearer that it's in curly braces whereas the "slot-wrapper" is not.

I think we could accomplish that by:

  1. Adding whitespace (newlines) after the opening { and before the closing }
  2. Adding a text node and/or id to the div that helps draw attention to it being in curlies:
Suggested change
{this.disabled && (
<div />
)}
{
this.disabled && <div id="in-curly-braces">Won't receive automatic key insertion</div>
}

☝️ That might not be the best in terms of presentation (particularly may be too wide for the viewport), but is the general idea. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm I think I'll go for something like

  { this.disabled && <div id="no-key">no key!</div> }

just to make sure it's narrow enough

@alicewriteswrongs alicewriteswrongs merged commit 30d26b1 into main Jan 26, 2024
4 of 5 checks passed
@alicewriteswrongs alicewriteswrongs deleted the ap/key-attr-docs branch January 26, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants