Juicy / imported-template

Custom element that lets you load template from external file into your document, and take full control over loaded <script>s and <link rel="import">s. Thanks to HTML Imports - caching, script execution, etc. is completely native. Declarative way for client-side includes boosted with HTML Imports features.
http://juicy.github.io/imported-template/examples/index.html
MIT License
28 stars 4 forks source link

Nodes stamped by dom-if are duplicated when imported-template is reattached to DOM #26

Open warpech opened 7 years ago

warpech commented 7 years ago

This is a continuation of to https://github.com/Starcounter/starcounter-include/issues/17, because after some research, I have managed to further isolate the problem to imported-template.

The problem is that nodes stamped by dom-if are duplicated when the parent node of imported-template is detached and then attached to DOM

I have written a test that fails in imported-template. I will commit it to a branch.

I think that the problem is between imported-template and Polymer, because using only Polymer this works as desired: https://jsbin.com/pesece/1/edit?html,js,output)

warpech commented 7 years ago

I did not manage to fix this, though I have isolated the problem in a single test. See the above commit.

This causes a problem in the Website app, meaning that it blocks the Launcher roadmap (https://github.com/StarcounterPrefabs/Launcher/issues/261, point 9)

@miyconst could you please take a look? Maybe a new pair of eyes can fix it.

screen shot 2017-01-17 at 18 47 52
warpech commented 7 years ago

I think I know what the problem is.

imported-template uses juicy-html's clear method, which only removes stampedNodes, but not scopedNodes nor scopelessNodes.

Overloading this method in imported-template using the following code fixes the problem.

I have made some test last month (commit e539d12) without a solution to the problem. Now I think I have the solution for the problem (see the snippet below). I am just not sure if it fixes the same problem or some other problem, but I am sure it is needed ;) Definitely the quality of it can be improved.

@tomalec, would you have time to check if this can solve the problem?

        var clear = JuicyHTMLElement.prototype.clear;
        ImportedTemplatePrototype.clear = function () {
            var childNo,
                scopedNodes = this.scopedNodes,
                len = scopedNodes && scopedNodes.length || 0;

            for (childNo = 0; childNo < len; childNo++) {
                clear.call({
                  stampedNodes: scopedNodes[childNo]
                });
            }
            this.scopedNodes = [];

            clear.call({
              stampedNodes: this.scopelessNodes
            });
            this.scopelessNodes = [];

            // inline HTML
            if (!this.scopedNodes && !this.scopelessNodes) {
              clear.call({
                stampedNodes: this.stampedNodes
              });
              this.stampedNodes = null;
            }
        }

Then, you also need to change:

                xhtml.scopedNodes = [];
                xhtml.scopelessNodes = [];
                xhtml.clear();

to:

                xhtml.clear();
tomalec commented 7 years ago

Nic catch! I'll check it tomorrow morning

tomalec commented 7 years ago

I think that the problem is between imported-template and Polymer, because using only Polymer this works as desired: https://jsbin.com/pesece/1/edit?html,js,output)

It seems your re-pro with only polymer, was checkign wrong thing. Take a look here https://jsbin.com/xiwuhogeho/1/edit?html,js,console,output

tomalec commented 7 years ago

I don't think your snippet is needed and could help somehow, as at https://github.com/Juicy/imported-template/blob/master/imported-template.html#L103 we are making stampedNodes contain all child nodes of stamped fragment therefore it should contain both scoped and scopeless as well.

Also it does not make tests https://github.com/Juicy/imported-template/tree/reattached-fix/test/use-cases/dom-manipulation pass.

warpech commented 7 years ago

Yeah, I just was able to reproduce the original problem by running Website + People + SignIn.

My snippet from https://github.com/Juicy/imported-template/issues/26#issuecomment-279549037 indeed does not help with that problem.

@tomalec any idea how to fix the OP?

tomalec commented 7 years ago

Ok, now I can reproduce it, I'll dig deeper

tomalec commented 7 years ago

I have reduced it a little bit: Website + People with two simple HTMLs + SignIn

So, the suspects are : imported-template, Polymer, People app

still investigating

tomalec commented 7 years ago

I have found something that could cause it https://github.com/StarcounterPrefabs/Website/issues/46, however imported-template should handle it anyway

tomalec commented 7 years ago

After some investigation I would bet it's related to this issue https://github.com/Polymer/polymer/issues/3682 And I would really like not to try fixing it or working it around in yet another place before Polymer update.

I suspect fixing https://github.com/StarcounterPrefabs/Website/issues/46 should hide this problem for some time.

@warpech WDYT?

un-tone commented 7 years ago

Fixing https://github.com/StarcounterPrefabs/Website/issues/46 does not hide the problem.

tomalec commented 7 years ago

Could you provide a build with that fix, so I could investigate further?

un-tone commented 7 years ago

@tomalec this PR https://github.com/StarcounterSamples/Website/pull/53 hides the problem

warpech commented 7 years ago

Get back to this in Starcounter 2.4