search

WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.07k stars 112 forks source link

>v1.80 no longer passes arrays and objects as properties to custom elements #119

Closed robdodson closed 7 years ago

robdodson commented 7 years ago

Hey @WebReflection, I was just updating the dependencies for custom-elements-everywhere and noticed hyperHTML started failing a couple of tests starting at version 1.8.0.

I'm not sure if this was an intentional change or not, so I wanted to raise an issue to see if you had more info.

WebReflection commented 7 years ago

not intentional, I should've put those tests in this build process. Thanks for filing the bug, will have a look ASAP.

WebReflection commented 7 years ago

Hi @robdodson here an update.

Latest 1.10.2 version is more forgiving and it sets values as both attributes and properties until the Custom Element gets promoted.

This works pretty well with native Custom Elements, and you can test it through the following test page:

<!doctype html>
<script src="https://unpkg.com/hyperhtml@latest/min.js">
/*! (c) 2017 Andrea Giammarchi @WebReflection, (ISC) */
</script>
<script>
/**
 * @license
 * Copyright 2017 Google Inc. All rights reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

class CEWithProperties extends HTMLElement {
  set bool(value) {
    this._bool = value;
  }
  get bool() {
    return this._bool;
  }
  set num(value) {
    this._num = value;
  }
  get num() {
    return this._num;
  }
  set str(value) {
    this._str = value;
  }
  get str() {
    return this._str;
  }
  set arr(value) {
    console.log(value);
    this._arr = value;
  }
  get arr() {
    return this._arr;
  }
  set obj(value) {
    this._obj = value;
  }
  get obj() {
    return this._obj;
  }
}

customElements.define('ce-with-properties', CEWithProperties);
</script>
<script>
const ComponentWithProperties = () => hyperHTML`
  <div>
    <ce-with-properties id="wc"
      bool=${true}
      num=${42}
      str=${'hyperHTML'}
      arr=${['h', 'y', 'p', 'e', 'r', 'H', 'T', 'M', 'L']}
      obj=${{org: 'viperHTML', repo: 'hyperHTML'}}
    ></ce-with-properties>
  </div>`;
</script>
<script>
this.onload = () => {
  const root = document.body;
  root.appendChild(ComponentWithProperties());
  let wc = root.querySelector('#wc');
  console.assert(
    JSON.stringify(wc.arr) ===
    '["h","y","p","e","r","H","T","M","L"]',
    'arr is correct'
  );
  console.assert(
    JSON.stringify(wc.obj) ===
    '{"org":"viperHTML","repo":"hyperHTML"}',
    'obj is correct'
  );
  setTimeout(() => {
    let wc = root.querySelector('#wc');
    console.assert(
      JSON.stringify(wc.arr) ===
      '["h","y","p","e","r","H","T","M","L"]',
      'arr is correct'
    );
    console.assert(
      JSON.stringify(wc.obj) ===
      '{"org":"viperHTML","repo":"hyperHTML"}',
      'obj is correct'
    );
  }, 10);
};
</script>

However, I think your Custom Elements polyfill gets on the way and the test might fail regardless.

This is not a hyperHTML responsibility so please help me figure out how to fix webcomponents polyfill, since it's not respecting standard behavior, thanks.

robdodson commented 7 years ago

I just pulled down 1.10.2 and it works with the tests on custom-elements-everywhere. At least I got a 30/30 when I tried :)

I don't think I know enough about the polyfill to understand which part may be failing. cc @bicknellr

WebReflection commented 7 years ago

never mind then, I think I've messed up my environment then. Although it was a funny bug to deal with for a while, thanks again for filing this.

@bicknellr most likely nothing to see here :wink:

WebReflection commented 7 years ago

Hi @robdodson , I've actually realized, after further investigation, I am cheating here. The problem is, that's most likely not just me, pretty much many other frameworks out there.

I believe the test, when it comes to el.arr and el.obj should also check that el._arr === el.arr and el._obj === el.obj.

I'd be surprised if hyperHTML would be the only one failing there, the unobservable Custom Element upgrade plays a very important role in that test.

Please update your tests verifying that prototype accessors are actually triggered. Just accessing el.arr or el.obj in that test is half of a lie.

I have both a PR that should fix this for hyperHTML, and I need to update the test in your repo too so that the check is performed asynchronously (after the upgrade of the CE which is never synchronous).

If you have any question, please feel free to ask in here, thanks.

robdodson commented 7 years ago

Is your concern that the element isn't upgraded when the framework sets the properties? If so, I think that's actually fine. We even have a pattern in the custom element best practices that shows folks how to handle that scenario in their elements. https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties

WebReflection commented 7 years ago

ehm no. my concern is that not a single setter/getter is accessed in that test. the CEWithProperties class prototype is never touched. You can test that by changing your repo as such:

  it('will pass array data as a property', function() {
    root.appendChild(ComponentWithProperties());
    let wc = root.querySelector('#wc');
    expect(wc.arr).toEqual(['h', 'y', 'p', 'e', 'r', 'H', 'T', 'M', 'L']);
    expect(wc.arr).toEqual(wc._arr); // this fails, it shouldn't
  });
WebReflection commented 7 years ago

please note this is most likely an issue with many libraries in there

edit anyway, late o-clock here, will continue tomorrow

WebReflection commented 7 years ago

Hi @robdodson , in case you are curious about the issue I am mentioning here, this is an issue I've opened in webcomponents repository: https://github.com/w3c/webcomponents/issues/671#issuecomment-332867127

robdodson commented 7 years ago

I think the problem you're describing in that thread is the one I mentioned previously. It's possible for a third party to set a property on a custom element before it is defined/upgraded and that instance property ends up shadowing the custom element's property. As a result the setter never triggers.

The workaround I've been suggesting is for custom element's to check if they have any shadowed instance properties at startup time. That's outlined in this section on "lazy properties". https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties

It's definitely an annoying workaround and I agree I would like for this to not be something that custom element authors or frameworks have to try to work around. If there's a solution available in the platform that would be great.