Fix: Repeat/Map now always rerender. #183

Kelketek · 2024-07-23T21:10:01Z

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 · 2024-07-23T21:12:18Z

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 · 2024-08-06T00:14:49Z

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 · 2024-08-06T00:18:44Z

x-element.js

+    if (typeof key !== 'function') {
+      throw new Error(`Unexpected map key "${key}" provided, expected a function.`);
+    }
+    return Template.map(items, identify, callback,  key,'map');
  }

  /** Shim for prior "repeat" function. Use "map". */


@theengineear is repeat a backward compatible thing? on the deprecation path in favor of map?

klebba · 2024-08-06T00:21:23Z

.gitignore

@@ -1 +1,2 @@
 node_modules
+.nvmrc


Nit: it's customary to add this type of thing to your workstation's global .gitignore because nvm is not a prerequisite of this lib

Thanks for this-- I had no idea that .gitignores above a repository root would be respected, but on reflection I can't think of why they wouldn't be. I'll remove this.

Edit: Haha, turns out they indeed wouldn't be. There's an extra step, git config --global core.excludesfile ~/.gitignore. Writing this for anyone else who learned it for the first time and finds this later.

klebba · 2024-08-06T00:22:13Z

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

Kelketek · 2024-08-06T13:35:49Z

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 #180 . I made the pull request against it because when you wrote the documentation there, you discovered #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 · 2024-08-06T13:40:22Z

@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 · 2024-08-08T22:02:02Z

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 · 2024-08-08T22:04:27Z

@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 · 2024-08-26T19:02:29Z

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

Kelketek · 2024-08-26T19:04:48Z

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

Kelketek · 2024-08-26T21:48:52Z

@klebba Rebased! :)

klebba · 2024-10-07T21:42:53Z

@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: #179 (comment)

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 Issue with built-in template repeat #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

Kelketek · 2024-10-07T22:03:59Z

Hi @klebba ! I'd love to see the comments and will see what I can do to address your concerns. I'd like to see this through.

klebba · 2024-10-07T20:57:37Z

x-element.js

    const reference = Template.#createWeakMapReference();
    const context = { identify, callback };
    const updater = (type, lastValue, details) => Template.#map(type, value, lastValue, details, context, name);
-    updater.value = value;
+    updater.value = key();


We think perhaps map should always be marked for re-render (and the call signature here would not change). It might be as simple as removing the guard here:

x-element/x-element.js

Line 1586 in e60005b

if (value !== lastValue) {

It could take some back and forth to get the behavior/spec/semantics right, but we could always take this up in a future change set. Hope that makes sense!

klebba · 2024-10-07T21:46:20Z

test/test-template-engine.js

@@ -532,6 +532,48 @@ describe('html updaters', () => {
    container.remove();
  });

+  it('repeat re-runs each time by default', () => {


Lets rework this so that it covers the repro case in #179 — e.g. the spec change should cover the user-facing confusion around trying to bind to changing outer context within map

klebba · 2024-10-07T21:48:13Z

test/test-template-engine.js

+    container.remove();
+  });
+
+  it('repeat respects a cache key', () => {


this spec change covers an implementation detail that we would prefer not to leak to the end user (in other words: we should not include this interface in the spec — map and repeat should not have changes to their call signature)

Kelketek · 2024-10-08T00:27:37Z

@klebba I've added a commit that removes the additional key function/interface change. Let me know if this looks good to you, and I'll squash these down so they're ready for merge. :)

theengineear · 2024-10-08T00:31:26Z

Thanks for re-writing that spec and updating the implementation — it’s much clearer to see the real-world requirement here 🤘

I’m going to go through and leave some nit-picky comments in there to make sure the changes land in a consistent way with proximal code.

Kelketek · 2024-10-08T00:47:09Z

Glad to hear it, @theengineear :) I'm heading off for the night but expect to clear out the nits tomorrow.

theengineear

Just some minor comments in here. At a high-level:

Fix up some stylistic nit-picks in here to make it match proximal code.
Pull documentation updates into a different PR.

Thanks again for driving this change 🤘

theengineear · 2024-10-08T00:33:20Z

test/test-template-engine.js

+      <div>
+        <ul id="target">
+          ${repeat(items, item => item.id, item => {
+            return html`<li id="${ item.id }">${ lookup?.[item.id] }</li>`;


super-nit: I think proximal code doesn’t put spaces around interpolations:

return html`<li id="${item.id}">${lookup?.[item.id]}</li>`;

(same goes for the other test)

theengineear · 2024-10-08T00:34:00Z

test/test-template-engine.js

+    const container = document.createElement('div');
+    document.body.append(container);
+    const items = [{ id: 'a' }, { id: 'b'}, { id: 'c' }];
+    let lookup = {a: 'foo', b: 'bar', c: 'baz'};


nit: Add spaces around object literals:

let lookup = { a: 'foo', b: 'bar', c: 'baz' };

(same goes for the other test)

theengineear · 2024-10-08T00:34:38Z

test/test-template-engine.js

+    assert(container.querySelector('#a').textContent === 'foo');
+    assert(container.querySelector('#b').textContent === 'bar');
+    assert(container.querySelector('#c').textContent === 'baz');
+    lookup = {a: 'fizzle', b: 'bop', c: 'fuzz'}


nit: Add spaces around object literals:

lookup = { a: 'fizzle', b: 'bop', c: 'fuzz' }

(same goes for the other test)

theengineear · 2024-10-08T00:42:50Z

doc/TEMPLATES.md

+```
+
+Another method might be to use an observation library to watch for changes to the objects you're iterating over, and then run the render function as a callback.
+


@Kelketek — I think what you’re trying to highlight here is that re-rendering is only triggered based on a change-by-reference detection (which is definitely by design).

I wonder if we could work this out in a separate documentation-focused PR? The implementation changes seem like they could be ready to merge pretty soon and I wouldn’t want to hold that up while we discuss the high-level documentation changes 🤙

@theengineear Removed-- and I'll create a follow-up PR for this, probably with some revised wording. Yes, that indeed is what I'm trying to point out, but I maybe I should phrase it differently.

@theengineear Started rewriting this, and decided I'd like to use the library a bit more before giving it another stab. I'll create the PR when I have a more complete view of what to recommend to the reader and how to frame it.

Kelketek · 2024-10-08T14:37:57Z

@theengineear I believe all nits are addressed (and I also found a couple of more since I realized eslint was set up here and I could run npm run lint-fix.)

Kelketek · 2024-10-08T14:55:24Z

@theengineear Just one more thing-- I've changed the base branch to main now that I'm no longer including documentation updates that depended on minor-housekeeping.

theengineear

LGTM! Thanks again for sticking with this one — apologies that it took us so long to circle back!

Kelketek · 2024-10-08T15:12:02Z

No worries, @theengineear . I had a great time!

@klebba Github indicates this is still waiting for your signoff. Did you want to give it one more look?

klebba

LG2M, thanks @Kelketek!

klebba requested review from klebba and theengineear August 6, 2024 00:13

klebba reviewed Aug 6, 2024

View reviewed changes

klebba force-pushed the minor-housekeeping branch from e027639 to a9acda7 Compare August 8, 2024 22:43

klebba mentioned this pull request Aug 9, 2024

Issue with built-in template repeat #179

Closed

Kelketek force-pushed the fox/template-repeat-issue branch from d221111 to fc7f6d9 Compare August 26, 2024 21:47

klebba requested changes Oct 7, 2024

View reviewed changes

Kelketek force-pushed the fox/template-repeat-issue branch 3 times, most recently from 1beedcc to 2ba0b42 Compare October 8, 2024 00:22

theengineear requested changes Oct 8, 2024

View reviewed changes

Kelketek force-pushed the fox/template-repeat-issue branch 2 times, most recently from 4e4883d to a5af069 Compare October 8, 2024 14:34

Kelketek changed the title ~~Fix: Repeat and Map can now rerender.~~ Fix: Repeat/Map now always rerender. Oct 8, 2024

fix: Repeat/map now always rerender. Resolves Netflix#179.

397b178

Kelketek force-pushed the fox/template-repeat-issue branch from a5af069 to 397b178 Compare October 8, 2024 14:53

Kelketek changed the base branch from minor-housekeeping to main October 8, 2024 14:53

theengineear approved these changes Oct 8, 2024

View reviewed changes

klebba approved these changes Oct 8, 2024

View reviewed changes

klebba merged commit ce67bc9 into Netflix:main Oct 8, 2024

Kelketek deleted the fox/template-repeat-issue branch October 8, 2024 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Repeat/Map now always rerender. #183

Fix: Repeat/Map now always rerender. #183

Kelketek commented Jul 23, 2024 •

edited

Loading

Kelketek commented Jul 23, 2024

klebba commented Aug 6, 2024 •

edited

Loading

klebba Aug 6, 2024 •

edited

Loading

klebba Aug 6, 2024

Kelketek Aug 6, 2024 •

edited

Loading

klebba commented Aug 6, 2024

Kelketek commented Aug 6, 2024

Kelketek commented Aug 6, 2024

klebba commented Aug 8, 2024

Kelketek commented Aug 8, 2024

klebba commented Aug 26, 2024

Kelketek commented Aug 26, 2024

Kelketek commented Aug 26, 2024

klebba commented Oct 7, 2024

Kelketek commented Oct 7, 2024

klebba Oct 7, 2024

klebba Oct 7, 2024

klebba Oct 7, 2024

Kelketek commented Oct 8, 2024

theengineear commented Oct 8, 2024

Kelketek commented Oct 8, 2024

theengineear left a comment

theengineear Oct 8, 2024

theengineear Oct 8, 2024

theengineear Oct 8, 2024

theengineear Oct 8, 2024

Kelketek Oct 8, 2024

Kelketek Oct 8, 2024

Kelketek commented Oct 8, 2024

Kelketek commented Oct 8, 2024

theengineear left a comment

Kelketek commented Oct 8, 2024

klebba left a comment

		```

		Another method might be to use an observation library to watch for changes to the objects you're iterating over, and then run the render function as a callback.

Fix: Repeat/Map now always rerender. #183

Fix: Repeat/Map now always rerender. #183

Conversation

Kelketek commented Jul 23, 2024 • edited Loading

Kelketek commented Jul 23, 2024

klebba commented Aug 6, 2024 • edited Loading

klebba Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kelketek Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

klebba commented Aug 6, 2024

Kelketek commented Aug 6, 2024

Kelketek commented Aug 6, 2024

klebba commented Aug 8, 2024

Kelketek commented Aug 8, 2024

klebba commented Aug 26, 2024

Kelketek commented Aug 26, 2024

Kelketek commented Aug 26, 2024

klebba commented Oct 7, 2024

Kelketek commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kelketek commented Oct 8, 2024

theengineear commented Oct 8, 2024

Kelketek commented Oct 8, 2024

theengineear 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

Kelketek commented Oct 8, 2024

Kelketek commented Oct 8, 2024

theengineear left a comment

Choose a reason for hiding this comment

Kelketek commented Oct 8, 2024

klebba left a comment

Choose a reason for hiding this comment

Kelketek commented Jul 23, 2024 •

edited

Loading

klebba commented Aug 6, 2024 •

edited

Loading

klebba Aug 6, 2024 •

edited

Loading

Kelketek Aug 6, 2024 •

edited

Loading