x-tag / core

The Heart of X-Tag
http://x-tag.github.io/
Other
1.25k stars 151 forks source link

this.innerHTML in V2 throws recursion error. #183

Closed ghost closed 6 years ago

ghost commented 6 years ago

I believe this is related to issue #182 (Recursion Error) I mentioned this in that thread but it should be worked on as a separate issue from the Recursion Error problem with using render(). I am not sure where to start on this one, but I've looked at the XTageElement's inheritance of the HTMLElement as a possible culprit.

Ok, this is the spec section 2.2 stuff I suspected. Today I talked with a couple Google friends and they said to do in-source testing because it was probably a difference between the parser flow and the dynamically created element flow. I need to move the onConstruct hook out of the constructor and figure something else out if we want to maintain the automatic construction/render part.

Do you think this could be related to the onConstruct() method being called inside the constructor() as well?

csuwildcat commented 6 years ago

Yeah, I think this is all related to the constructor issue. I'll figure out a different strategy this weekend.

ghost commented 6 years ago

Hi @csuwildcat, thought you'd like to see the core-2 jasmine test on an asp server.

Let me know if the link doesn't work. --EDIT [It uses the updated jasmine test].
https://iult.worldsecuresystems.com/node_modules/core-2/tests/basic/index.html

ghost commented 6 years ago

I've got it throwing a different error, by changing the getOptions() method. I think if we filtered the object assigning, it will work to resolve this issue. I will hopefully run a test with a new build in the next couple of days with a filtered getOptions()

ghost commented 6 years ago

Just to be sure, are people running V2 without the shadow patch? Because I am...

WARNING: 
Custom Elements: `Element#attachShadow` was not patched. 
csuwildcat commented 6 years ago

I am not including the shadow DOM polyfill, as it is very spotty due to certain native hooks/aspects that can't be truly polyfilled.

On Oct 12, 2017 9:41 PM, "KIP" notifications@github.com wrote:

Just to be sure, are people running V2 without the shadow patch? Because I am...

WARNING: Custom Elements: Element#attachShadow was not patched.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/x-tag/core/issues/183#issuecomment-336348762, or mute the thread https://github.com/notifications/unsubscribe-auth/AAICyhwWkqsIDVmaxOSxXiCnZaswkTJPks5srunzgaJpZM4PxXaK .

ghost commented 6 years ago

Okay, I thought so.

The next question is of course, should I run the shadow DOM poly ... and of course that is really only a quick fix maybe ... I'm not really certain about what can really be done.

These constructor classes are some seriously cool things to play around with, but are mercilessly temperamental. Manipulating the this object from xtags could be a step but I don't believe you meant xtags to take the place of the custome-elements?

Testing out the shadow DOM poly seems sensible because somebody out their will undoubtedly do this of their own accord so maybe I'll just do that and see what happens.

csuwildcat commented 6 years ago

I have code being checked in tomorrow that I hope fixes all of this. I'll message back here the details when I push it.

PikioopSo commented 6 years ago

I was building a version dashboard for this issue. Did you want a link?

-was thinking of pushing a custom-elements dashboard built with x-tags/jasmine V2 to Pi Reel.

PikioopSo commented 6 years ago

The goal of it was to track different XTagElement Object Constructors that maybe an acceptable alternative.

PikioopSo commented 6 years ago

Cause that's where I was going to make a change to try to get this.innerHTML back into the mix.

csuwildcat commented 6 years ago

OK, can you pull the latest V2 branch and test against it? I have added a few things:

When you declare a template in the Class definition, it is no longer stamped out in the constructor to avoid the issues with mods to children/attributes that cause CE upgrades to fail (per the spec).

Template creation and injection is now done async. By default this happens the first time the connectedCallback fires, or, if you specify create as an arg in your template definition, the template creation/injection is put into a lifecycle flow internal to X-Tag that kicks off from the constructor call and completes sometime after construction. There is a Promise you can use to act on your element when it is fully created and ready. Here's an example of all this:

// This component will have its template created and injected in the connectedCallback:

    var component1 = xtag.create(class extends XTagElement {
      '::template(true)'(){
        return `<h1>title auto 1</h1><p>content auto 1</p>`;
      }
    });

// This component will have its template created and injected in X-Tag's custom created flow:

    var component2 = xtag.create(class extends XTagElement {
      '::template(true, create)'(){
        return `<h1>title auto 2</h1><p>content auto 2</p>`;
      }
    });

// You can wait to act on your element at the end of the created lifecycle by using whenReady() 

    component2.whenReady(function() {
      // component should have all attributes, children, and setup done
    });
ghost commented 6 years ago

I will use the new build tonight, when I setup the outerHTML tests.

ghost commented 6 years ago

I just ran the index file and all tests went through. Thanks so much for this update!

:)

ghost commented 6 years ago

Does this close the issue?

ghost commented 6 years ago

All of the x-tags API is working normally as far as I know right now according to the test, but the use of this.innerHTML still throws the recursion error.

Get back to you on the outerHTML soon.

csuwildcat commented 6 years ago

Can you paste in the single, reduced test case where you are touching this prop and experiencing this?

On Oct 14, 2017 7:00 PM, "KIP" notifications@github.com wrote:

All of the x-tags API is working normally as far as I know right now according to the test, but the use of this.innerHTML still throws the recursion error.

Get back to you on the outerHTML soon.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/x-tag/core/issues/183#issuecomment-336680505, or mute the thread https://github.com/notifications/unsubscribe-auth/AAICylQhDxu_FZv-cX56_ZPXUZ50n3Y5ks5ssWdRgaJpZM4PxXaK .

ghost commented 6 years ago

I'll get a link to it up in a bit.

EDIT: Sorry I misunderstood. The pasted code is below

ghost commented 6 years ago

Okay, I'm going to use two scenarios one passing with no recursion when this.innerHTML is used and one that is. The failing build is caused by wrapping this.innerHTML inside the constructor.

ghost commented 6 years ago

The below xtag element doesn't throw a recursion error.

xtag.create("x-testa", class extends XTagElement {
    constructor(){
        super();
        this.hello = "BOO";
    }
    set 'hello::attr'(a){
        this.innerHTML = a;
        return "world";
    }
} );
ghost commented 6 years ago

The below xtag element does throw a recursion error.

xtag.create("x-testa", class extends XTagElement {
    constructor(){
        super();
        this.innerHTML= "BOO";
    }
    set 'hello::attr'(a){
        // blah definition
        return "world";
    }
} );
csuwildcat commented 6 years ago

Yeah, you can't touch child nodes or attributes (or anything that changes them) in the constructor - that's a violation of the Custom Elements spec, not an X-Tag bug: https://w3c.github.io/webcomponents/spec/custom/#custom-element-conformance

On Oct 14, 2017 8:31 PM, "KIP" notifications@github.com wrote:

The below xtag element does throw a recursion error.

xtag.create("x-testa", class extends XTagElement { constructor(){ super(); this.innerHTML= "BOO"; } set 'hello::attr'(a){ // blah definition return "world"; } } );

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/x-tag/core/issues/183#issuecomment-336683774, or mute the thread https://github.com/notifications/unsubscribe-auth/AAICyhcSNTusvzh-SjqVf0zUDaghnRs2ks5ssXyZgaJpZM4PxXaK .

ghost commented 6 years ago

Yeah I thought so...

-- it's already reported on webcomponets repo so it probably should be closed here. -- and again thanks for all the work you do with xtags.

ghost commented 6 years ago

I have a question though?

xtag.create("x-testa", class extends XTagElement {
    constructor(){
        super();
        this.innerHTML = "BOO";
    }
    set 'innerHTML::attr'(a){
        this.innerHTML = a;
        return "world";
    }
} );
ghost commented 6 years ago

Is something like the above happening in custom elements? Where they create a property and setter and use a constructor to initiate it? Or in this case when people write scripts behind it the recursion error happens.

csuwildcat commented 6 years ago

The attribute route works because X-Tag is routing that set through the attributeChanged callback, which is async outside of the constructor's execution. Thus it doesn't violate the spec rules.

On Oct 14, 2017 9:08 PM, "KIP" notifications@github.com wrote:

Is something like the above happening in custom elements? Where they create a property and setter and use a constructor to initiate it? Or in this case when people write scripts behind it the recursion error happens.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/x-tag/core/issues/183#issuecomment-336685017, or mute the thread https://github.com/notifications/unsubscribe-auth/AAICyirPzlvUEluFGZPM6l9SP6N7jWwRks5ssYU9gaJpZM4PxXaK .

ghost commented 6 years ago

This really isn't a simple topic, as far as pragmatic schemes, but I can see what you mean...It's like a single stream that behaves like two on their own paths?

ghost commented 6 years ago

Do you know about the outerHTML bug? If it's a real bug I think that would be custom-elements too.

ghost commented 6 years ago

I was doing more testing and didn't get the undefined response instead I got null, but that could be because I was executing it with a setter.

ghost commented 6 years ago

Found an issue referencing element.outerHTML but they aren't reporting any null or undefined return values.

https://github.com/webcomponents/custom-elements/issues/71

ghost commented 6 years ago

Setter for template doesn't have a return value so a template attribute is created with the value undefined.

 set 'template::attr' (name){
   var _name = name || 'default';
   var template = this.templates[_name];
     if (template) {
        var node = range.createContextualFragment(template.call(this));
        this._templateNode = this._templateNode ? this.replaceChild(node, this._templateNode) && node : this.appendChild(node);
        this._templateRender();
      }
      else throw new ReferenceError('Template "' + _name + '" is undefined');
}
ghost commented 6 years ago

Do you want this pull request?

 set 'template::attr' (name){
   var _name = name || 'default';
   var template = this.templates[_name];
     if (template) {
        var node = range.createContextualFragment(template.call(this));
        this._templateNode = this._templateNode ? this.replaceChild(node, this._templateNode) && node : this.appendChild(node);
        this._templateRender();
      }
      else throw new ReferenceError('Template "' + _name + '" is undefined');
return _name;
}
ghost commented 6 years ago

Promise <value> is undefined as well, I passed name to the _renderTemplate() method.

ghost commented 6 years ago

In regards to the specs of x-tags V2, are you supposed to be able to use render() only once for each custom element or is it supposed to render when ever called?

At the moment each element only renders one template each.