x-tag / core-2

X-Tag 2 - A tiny Web Components library that packs a big punch
MIT License
37 stars 8 forks source link

::template(true) -- too much recursion #3

Open dspinhirne opened 7 years ago

dspinhirne commented 7 years ago

Looking at your tests as examples for using the code and ran into an issue with templates. From your tests is seems that providing the "true" arg to a template should cause it to render automatically, however in practice this leads to a recursion error being raised. Using code from your basic test case, registering it, and using it in an html file:

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

--
<x-component1></x-component1>

Raises this issue in the js console.

csuwildcat commented 7 years ago

Looking into this now.

csuwildcat commented 7 years ago

@dspinhirne I can't seem to raise this error when I run the tests in any browser I am testing with. Are you saying it is behaving differently outside of the test spec?

dspinhirne commented 7 years ago

Yes, this is outside the test spec. I am attempting to use templates as part of one of my own custom components and hit the issue.

In order to verify my own work I used code from your tests and hit the same issue. Basically, I created a js file and copied the code for one of your testing custom components into it. I then created an html file and attempted to use the custom element (x-component1) as shown above. This is where I hit the issue.

ghost commented 7 years ago

I'm sorry this is late @csuwildcat but I ran into the same problem with using templates.

@csuwildcat and @dspinhirne, I've got it to work, here is the code below.

var _temp = xtag.create(class _temp extends XTagElement { 'hello::template(true)'(){ return <h1>title1</h1><p>content1</p>; } });

var Fire = xtag.create(class Fire extends XTagElement{ connectedCallback (){ console.log("hi"); this.render("hello"); } } );

xtag.register("fire-base", Fire);

ghost commented 7 years ago

@csuwildcat I guess my only question is, what does the true parameter do? In your specs.js file it says:

"auto-render templates marked with the option", func...

Is this supposed to work like v1's content: property?

csuwildcat commented 7 years ago

In prep for release, I started committing to the v2 branch in the core repo. This one is behind a few commits.

About the template extension: it's for stamping out child content, and the true parameter sets it to automatically do so when an element is created. Can you run the Core v2 tests and tell me what browser and platform it's failing in? For me it works in all browsers on Windows, and I'm going to test Mac this week.

On Sep 17, 2017 7:51 PM, "KIP" notifications@github.com wrote:

@csuwildcat https://github.com/csuwildcat I guess my only question is, what does the true parameter do? In your specs.js file it says:

"auto-render templates marked with the option", func...

Is this supposed to work like v1's content: property?

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

ghost commented 7 years ago

Yeah no problem, @csuwildcat

Did you want me to test the content at https://github.com/x-tag/core/tree/v2 -- edit below

I opened the index.html file at,
https://github.com/x-tag/core-2/blob/master/tests/basic/index.html in this repository on my computer, is that what you mean by testing? I apologize if I'm misunderstanding.

If that's what you meant all those tests are clicking good, the recursion problem only occurs during the xtag.register() of a custom element (The tests inside, specs.js test the xtag.create() method but not template registrations.)

The ::template() method works if you just use the create method and give the template a name, and than manually use it with this.render(yourTemplate) in other elements.

When I tried to debug the issue using the constructor() method I got over 1000 console logs and than finally got the recursion error.

I don't know if any of this helps, but I'll keep plucking away at it.

ghost commented 7 years ago

Tested out using the template again just now. It's still giving me a recursion error on Firefox 64 bit Nightly build most current edition, and is also giving me a similar error on MS Edge except it says out of out of stack space of course.

On Opera I get an error that points to the file containing the element with the error, Uncaught DOMException: Failed to construct 'CustomElement': The result must not have children.

-edit

Also, the errors I'm getting aren't from xtag source but from custom-elements.min.js

ghost commented 7 years ago

Could this happen if you haven't patched the shadowDOM or polyfill? If it is please close the issue. Thank you.

@dspinhirne are you still having this problem. If you are just use the cdn to quickly resolve it or I think checking to make sure all the files in bower are installing correctly. @csuwildcat, does this sound correct?

Great job @csuwildcat

ghost commented 7 years ago

I was just wondering @csuwildcat have you ever thought of switching to yarn?

csuwildcat commented 7 years ago

NPM 5+ has locking and tree shaking, is that the sort of thing you were looking for in using Yarn?

On Sep 30, 2017 7:55 AM, "KIP" notifications@github.com wrote:

I was just wondering @csuwildcat https://github.com/csuwildcat have you ever thought of switching to yarn?

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

ghost commented 7 years ago

Lol, no sorry it was just a personal thing. I was looking at Yarn and thought it was neat and clean. Was also wondering what the advantages would be exactly myself? Package locking I can see how can be useful but haven't really used it much myself

csuwildcat commented 7 years ago

I think this is probably related to child insertion inside the class constructor, which I can change. The issue I have is that none of my tests fail in Chrome, Firefox, or Edge on Windows - can you provide a failing test setup?

ghost commented 7 years ago

I was heading that way to, i wilk provide a build that fails on top of the xtag github site I havr forked when I get off work if that works for you.

csuwildcat commented 7 years ago

Yeah, that would help. I'm trying to make it fail, but can't seem to.

On Sep 30, 2017 9:53 AM, "KIP" notifications@github.com wrote:

I was heading that way to, i wilk provide a build that fails on top of the xtag github site I havr forked when I get off work if that works for you.

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

ghost commented 7 years ago

Im not on my comp now or i would push the failing build now I will push the failing build when

ghost commented 7 years ago

I get off, but I set to the script src "without cdn" and without the "./"

ghost commented 7 years ago

In the index.html file in the site repo. It failed for me.

ghost commented 7 years ago

Link to the repo I am using is here -> https://github.com/KipOmaha/x-tag-setup.

To get the build running you'll have to clone it (sounds wierd cause it don't run), but... I decided to create a GH-page for it and it throws a different error.

ghost commented 7 years ago

Link below repeated the error. Check commit message for info at, https://github.com/KipOmaha/x-tag-setup/commit/d7eaf3b140c3de99c1e2b8a6c954248a9121b88b

https://kipomaha.github.io/x-tag-setup/index.html

ghost commented 7 years ago

The construcor's error I believe was preventing the recursive error from appearing, @csuwildcat.

ghost commented 7 years ago

@csuwildcat, just wanted to update this thread.

I have a build with no recursion error and an altered render() method with no weird behaviors. (Testing in progress)

csuwildcat commented 7 years ago

Can we move all these Issues to the regular Core repo? That is where the most up-to-date code is, and I want to try closing this repo out.

I don't see errors on this page in any of my browsers on Windows (Edge, Chrome, Firefox, etc.): https://kipomaha.github.io/x-tag-setup/index.html.

Were you able to generate a reduced test case? Preferably, we could isolate this down to a single test we can add to the test spec that would throw if this issue was present.

ghost commented 7 years ago

@csuwildcat I can create a new jasmine.html with a reduced test, or I can add it direct to the spec.js test in the basic directory as a pull request to check for the issue, but at the moment I'm using a test library with a modified render method that gets rid of the error.

If you want this issue in core I would reccomend changing it to

recursion errors regarding this.innerHTML

This is where I`m finding recursion errors at least but it needs more testing.