Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

template binding alters script template block HTML #2484

Closed
bmsmg opened this issue Apr 30, 2019 · 15 comments
Closed

template binding alters script template block HTML #2484

bmsmg opened this issue Apr 30, 2019 · 15 comments

Comments

@bmsmg
Copy link

bmsmg commented Apr 30, 2019

I just tried an upgrade to 3.5 and noticed that using the 'template' binding with a named script template results in the script templates HTML being erased. See this fiddle: https://jsfiddle.net/rwL6pbmd/1/

It is a very simple example using a named template binding with a single data item. The issue is also present when using an array in foreach.

You'll notice that the script with id == 'tmpl-test' is originally populated with some dummy HTML, but that HTML gets erased after binding. Open your browser console to see the console.log messages.

The issue does not occur in 3.4.2 (my previous version).

@mbest
Copy link
Member

mbest commented Apr 30, 2019

Can you describe why it's a problem that the script's content is cleared?

@bmsmg
Copy link
Author

bmsmg commented Apr 30, 2019

Yes, my current issue is that the template is accessed from multiple modules at various times throughout my app's lifetime. The first time it gets accessed by the template binding it is cleared, so any subsequent attempts to retrieve it will return an empty string, causing elements for the templated type to have no content.

@mbest
Copy link
Member

mbest commented Apr 30, 2019

If you're accessing the template from another template binding it should work just fine since Knockout keeps an internal cache of the template contents. But I'll look into improving this for the next release.

@bmsmg
Copy link
Author

bmsmg commented Apr 30, 2019

This template is used by both the ko binding and in a more generic form where it becomes the basis for another HTML structure in pure javascript. Thanks.

@avickers
Copy link

Leaving the innerHTML intact would result in unnecessary overhead, no? Maybe a util function that takes a template name and returns a cloned nodelist from the cache?

@rcollina
Copy link

rcollina commented May 1, 2019

Thought I'd chime in, I've been puzzled by this behaviour before and realized that it was by-design from source not long ago. I believe the template should not be cleared.

@bmsmg
Copy link
Author

bmsmg commented May 1, 2019

Agreed. I'm not sure why it would seem desirable/acceptable to clear the template HTML. Here is the same example running with version 3.4.2: https://jsfiddle.net/r3sgw72j/

The template is left intact.

@mbest
Copy link
Member

mbest commented May 1, 2019

In 3.5.0, we changed the way named templates are used. Instead of parsing the string contents every time the template is used, we parse it once and save the parsed nodes. We wanted to maintain backwards compatibility, though, with the ability to change the template contents at runtime. So that means we need to be able to know that the template contents have been updated. It seemed easy and efficient to simply be able to check that the template is non-empty.

@avickers
Copy link

avickers commented May 1, 2019

For clarification, it is only possible to alter the template prior to applyBindings being called in either 3.4.2 or 3.5.

If you do this

ko.applyBindings({
  item: new Item()
});

document.getElementById('tmpl-test').innerHTML ='<span>Muhaha</span>'

console.log(script.innerHTML || '<empty node>');

Knockout does not update the DOM, so the template binding is still not dynamic in the way that the HTML binding is, for instance. Having it be only partially dynamic seems like the single most confusing outcome, from my perspective.

Polymer made a Mutation Observer polyfill that goes back to IE9. I know that you would probably be loathe to consider it, but MO would make it possible to check for changes to the template and even easily make templates fully dynamic--eliminating the ambiguity. FWIW, IE 9 and 11 are the only versions with non-zero market share for years and Microsoft End of Life'd all but IE11 in 2016.

https://github.com/webmodules/mutation-observer

@rcollina
Copy link

rcollina commented May 1, 2019

@mbest I’d vote for a non-mutating alternative to template management.

Perhaps tagging the template dom node is a good enough alternative to the non-empty check?

@avickers
Copy link

avickers commented May 1, 2019

I support immutable templates, as well. I do think the documentation needs to be explicit on this point, especially if the innerHTML hangs around.

I also wonder if there ought not be a way to flush particular templates from the cache? In an arbitrarily complex application, it might not be desirable to hold all templates in memory at all times, especially with the potential for duplication between the DOM tree template and the cached NodeList(?) representation.

@marcinkowal2015
Copy link

I also vote for immutable templates, in can also add that in my case problem appeared due to fact that we have some generic code which is rendering components and it's taking use of ko.renderTemplate. After upgrade to 3.5.0 first call with script template removed content of template, and next usages of same templates result in error(as we share some templates between multiple components).

@mbest
Copy link
Member

mbest commented Sep 29, 2019

@marcinkowal2015 Can you provide an example of this problem? Just based on my understanding of the Knockout code, I'm not able to understand a scenario that would reproduce what you're seeing.

@mbest
Copy link
Member

mbest commented Sep 29, 2019

I also wonder if there ought not be a way to flush particular templates from the cache?

Knockout uses its domData API to store the cached nodes for the template. The cache data for that template will be removed If the template node is cleaned up using ko.removeNode or ko.cleanNode.

@mbest mbest closed this as completed in 362b87a Sep 30, 2019
@mbest
Copy link
Member

mbest commented Sep 30, 2019

Thanks for all of the feedback. I've uploaded a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants