Fix: Repeat/Map now always rerender.

Kelketek commented 3 months ago

Resolves #179

Previously, the repeat and map functions were unable to rerender their contents at all unless the reference to the value they're iterating over changed. This change makes it to where, by default, repeat and map will run their template every time the template function renders, and adds a feature for memoizing the output with a cache key function if desired.

This merge request also adds documentation about the limitations of reactively re-rendering based on arrays and other complex data, and details workarounds/strategies for dealing with this.

As one more change, I've added .nvmrc to gitignore so that people using it don't pollute the repo. If desired, I could instead add the target npm version to its contents.

Testing instructions:

Create this element that leverages repeat:

import XElement from '../../x-element.js';

const entries = [{id: 'foo', value: 'FooValue', text: 'FooText'}, {id: 'bar', value: 'BarValue', text: 'BarText'}]

class MySelect extends XElement {
  static get properties() {
    return {
      selectedId: {
        type: String,
        default: 'foo',
        reflect: true,
      },
      options: {
        type: Array,
        // Will run once on load and then never again unless the array is replaced.
        observe: () => console.log('I ran!'),
        default: () => entries,
      },
    };
  }

  static template(html, {repeat}) {
    return ({ options, selectedId }) => {
      return html`
      <div>${selectedId}</div>
      <div>${JSON.stringify(options)}</div>
      <select name="my-options">
        ${repeat(options, option => option.id, option => html`
          <option value="${option.value}" ?selected="${option.id === selectedId}">${option.text}, ${selectedId}</option>
        `)}
      </select>
    `;
    };
  }
}

customElements.define('my-select', MySelect);
// We will edit this in the console.
window.entries = entries

Index:

<!doctype html>
<html>
  <head>
    <meta charset="UTF-8">
    <link rel="stylesheet" href="index.css">
    <script type="module" src="index.js"></script>
  </head>
  <body>
    <div>
      <my-select></my-select>
    </div>
  </body>
</html>

Load this new demo up and access the console. Edit the entries array and note that no re-rending happens, as explained in the new documentation.
let thing = document.querySelector('my-select')
thing.selectedId = 'bar'
Note that the select/options update, as well as the JSON value and the selected ID value at the top of the component.
Edit the original component to add a key function after the callback function for repeat, () => entries.
Refresh the page. Edit the entries again, and edit the selectedId again.
Notice that the component re-renders, with the new JSON value, and the new selectedId shown, but the select/ioptions remain unchanged, because the key has not changed (it's a reference to the existing list.)
Try different key function outputs to make the re-render work or not work.
npm run test to run the test suite, or let CI work its magic.

Author notes and concerns:

I think this approach is solid overall, but it raises a few questions. First is that there could be performance impacts by making repeat/map render all the time by default. I could change the default to NOT rendering all the time, though I think this behavior would be more surprising than the current one.

The second is that repeat appears to be begrudgingly supported right now, and I've had to add a good bit of repeated code to continue supporting it. This might be a good argument to remove it entirely while we're here, but I don't know what the downstream impact would be for Netflix.

The third is that adding the key function changes the function signature (albeit in a backwards-compatible way) of repeat and map, and these signatures are already a little long.

The fourth is I don't know the code which uses this code-- so I don't have a strong sense of how often repeat and map are used in practice, and whether instead of adding a cache key to repeat/map, I should instead make it to where they always re-render, and instead add some kind of memoization function that can be embedded into the template arbitrarily. Let me know if you'd prefer I do that, instead.

Finally, I'm new to web components and this library-- I only started reading up on the both of them a few days ago in my spare hours. So there's a chance I'm missing something that would be obvious to someone who has worked with them further. Seeing as the issue has been open a while, I figured I'd take a crack at it anyhow :)

Kelketek commented 3 months ago

Ping @klebba who was affected by this issue, and should be able to confirm if I understood it correctly and have resolved it satisfactorily.

klebba commented 3 months ago

Hi @Kelketek — thank you for taking this on. We will take a look at get back to you; just to set your expectations I would anticipate this taking a few weeks due to Summer vacations and various internal ongoings.

One question: did you intend to merge to minor-housekeeping or main? I think main is the more suitable branch, though I can't recall what minor-housekeeping is

klebba commented 3 months ago

Also FYI — I gave this a cursory glance but still need time to complete a more thorough review — thanks!

Kelketek commented 3 months ago

Hi @Kelketek — thank you for taking this on. We will take a look at get back to you; just to set your expectations I would anticipate this taking a few weeks due to Summer vacations and various internal ongoings.

One question: did you intend to merge to minor-housekeeping or main? I think main is the more suitable branch, though I can't recall what minor-housekeeping is

@klebba minor-housekeeping is https://github.com/Netflix/x-element/pull/180 . I made the pull request against it because when you wrote the documentation there, you discovered https://github.com/Netflix/x-element/issues/179. minor-housekeeping contains the markdown file where this information seemed most pertinent. Seemed best to make the PR against it to both make sure I had a good place to put the info and help you get the other PR across the finish line, since I figured adding docs that would cause the reader to run into a bug was part of the hold-up. :)

Kelketek commented 3 months ago

@klebba Thanks for the initial pass-- I've addressed the one particular note you had for me so far, and look forward to the full review. I'll wait on squashing 'til that's complete. :)

klebba commented 3 months ago

Got it — thanks for the context @Kelketek — would it be problematic for you if we were to merge minor-housekeeping ahead of this changeset?

Kelketek commented 3 months ago

@klebba No, not if you believe that minor-housekeeping is safe to merge without this change. If you do merge minor-housekeeping ahead of this changeset, I will rebase this PR against master instead.

klebba commented 2 months ago

@Kelketek just pinging here to let you know that we haven't forgotten about this. Thanks for your patience

Kelketek commented 2 months ago

@klebba You're welcome! I see I've got some conflicts now as well so I'll rebase when I get a minute.

Kelketek commented 2 months ago

@klebba Rebased! :)

klebba commented 1 month ago

@Kelketek — thanks for your continued patience. @theengineear and I met today to thoroughly review these proposed changes. Great work! We also revisited #179 to generate a reproduction case for the original issue: https://github.com/Netflix/x-element/issues/179#issuecomment-239792845

After some discussion we think that the XElement map directive is essentially memoizing input values when it should not.

We will leave a few comments on this pull request for your consideration, but also want to set your expectations:

to address #179 we do not believe an interface change will be necessary (will point out where in the PR this would need adjustment)
there is no pressure on you to fix this if your priorities have changed; just let us know and we can take it up in our next release