Netflix / x-element

A dead simple starting point for custom elements.
Apache License 2.0
28 stars 12 forks source link

Test “initial” works as expected with non-literal. #90

Closed theengineear closed 3 years ago

theengineear commented 3 years ago

When “initial” is provided with a non-literal (compound) value, the return value should never be shared across other instances of the element.

Closes #89

theengineear commented 3 years ago

@klebba — I'm having a hard time reproducing this issue in a test. Can you help be understand how this test isn't proving that x-element functions the way you want?

klebba commented 3 years ago

@theengineear Thanks for checking this out; seems I need to take another look!

klebba commented 3 years ago

Repro case:

<!doctype html>
<html>
    <head>
        <title>x-element initial</title>
        <style>:root { color-scheme: dark }</style>
    </head>
    <body>
        x-element issue <a href="https://github.com/Netflix/x-element/issues/89">#89</a>
        <div>
            <x-foo></x-foo>
            <x-foo></x-foo>
        </div>
        <script type="module">
            import XElement from 'https://cdn.jsdelivr.net/npm/@netflix/x-element@1.0.0-rc.43/x-element.js';

            class Baz {
                constructor() {
                    this.id = Math.floor(Math.random() * 100);
                }
            }

            export default class XFooElement extends XElement {
                static get properties() {
                    return {
                        bar: {
                            type: Baz,
                            internal: true,
                            initial: () => new Baz(),
                        },
                    };
                }

                static template(html) {
                    return ({ bar }) => html`
                        <div id="container">${ bar.id }</div>
                    `;
                }
            }

            customElements.define('x-foo', XFooElement);
        </script>
    </body>
</html>
theengineear commented 3 years ago

Ah... well, it would help if we released x-element. This has been fixed, but we were sorta waiting for lit 2.0 to happen — it never did... I'll release today.

This explains why my test case (running against latest master) didn't pick up this behavior but your example (running against the latest release) does!

theengineear commented 3 years ago

I'll release today so you can pull in the newest behavior.

theengineear commented 3 years ago

@klebba — Gonna merge this in since I think it's a good test case to have locked down.