BorisMoore / jsrender

A lightweight, powerful and highly extensible templating engine. In the browser or on Node.js, with or without jQuery.
http://www.jsviews.com
MIT License
2.68k stars 340 forks source link

Reloading JsRender removes the template that was cached on the script element #263

Closed nariman-haghighi closed 9 years ago

nariman-haghighi commented 9 years ago

Hi Boris

There's a new error in Commit 64 (Post Beta) that goes away as soon as you revert back one version. It happens even with the simplest of templates (a single div with no binding expressions) on line 192 of the formatted version:

if (t === !0 ? (n = t, t = void 0) : typeof t !== wt && (t = void 0), (l = this.tag) ? (d = this, p = l._.tmpl || d.tmpl, g = g || d.view, arguments.length || (e = g)) : p = this, p) {

Please let me know if this helps.

Cheers,

BorisMoore commented 9 years ago

Thanks Nariman - but that is strange, I have not seen that. Can you create a jsfiddle example which shows it. And can you use the non-minified version of JsRender? Thanks very much.

nariman-haghighi commented 9 years ago

We're not able to reproduce it in a jsfiddle yet but this is the exact line that generates the error on the non-minified version:

        if (tag = this.tag) {
            // This is a call from renderTag or tagCtx.render(...)
            tagCtx = this;
            tmpl = tag._.tmpl || tagCtx.tmpl;
            view = view || tagCtx.view;
            if (!arguments.length) {
                data = view;
            }
        } else {
            // This is a template.render(...) call
            tmpl = this;
        }

Some additional context, the call to $("...").render({}) works on the initial page load, but subsequent calls involving the same tag context generate the error above (after the document has fully loaded). The jsrender.js is lazy loaded and likely the reason why it works on initial page load but fails later on in response to user actions that trigger $("...").render({}).

Our code-base has been stable and works on all previous versions except the latest.

BorisMoore commented 9 years ago

OK - thanks.

In commit 63, $("...").render({}) goes through the code:

function $fastRender(data, context, noIteration) {
    var tmplElem = this.jquery && (this[0] || error('Unknown template: "' + this.selector + '"')),
        tmpl = tmplElem.getAttribute(tmplAttr);

    return fastRender.call(tmpl ? $templates[tmpl] : $templates(tmplElem), data, context, noIteration);
}

In commit 64 it goes through the code:

$.fn.render = function(data, context, noIteration) {
    var tmplElem = this.jquery && (this[0] || error('Unknown template: "' + this.selector + '"')),
        tmpl = tmplElem.getAttribute(tmplAttr);

    return renderContent.call(tmpl ? $templates[tmpl] : $templates(tmplElem), data, context, noIteration);
};

So essentially the same code. But the error you are getting might occur if tmpl ? $templates[tmpl] : $templates(tmplElem) is in fact null or undefined. In fact on the first call, tmplElem.getAttribute(tmplAttr); will return null, and the $templates(tmplElem) will therefore compile the template. On the second call, tmpl will be a string, and $templates[tmpl] should be the cached compiled template. It sounds as if on the second call the $templates[tmpl] does not in fact contain the cached template. Are you doing anything that might delete that cached template? Can you try and debug with breakpoints or console logging and try to see if there is an issue showing up in the above code? What does you lazy loading do, and does it affect this scenario somehow?

Otherwise I would need repro in some form (html files etc.) so I can debug this...

nariman-haghighi commented 9 years ago

We've isolated the behaviour here: https://jsfiddle.net/4rpqyk0g/1/

Our solution reloaded jsrender.js in between invocations, that's what causes the change between 63 and 64. We can work around this by avoiding reload if the following is true:

typeof $.templates != "undefined" || typeof jsviews != "undefined"
BorisMoore commented 9 years ago

Yes, that makes sense. In version 63 and previous, JsRender detected that a copy of JsRender had already been loaded, and in that case aborted the loading of a second instance. But with commit 64 support was added for AMD script loaders - which do not play well with trying to magically avoid loading a second instance, so now if you choose to load a new instance, then the new instance will indeed be loaded as expected...

If you are not using jQuery then commit 64 provides a noConfict() method to optionally keep global references for the previous instance. But if jQuery is loaded, then the second instance will replace $.templates, $.render etc. so you will lose your previous template registration.

So you need to choose whether to load a new instance and clear the previously registered templates, or not to load.

The next update (65) will use a different approach to caching templates from script blocks, so in fact your scenario above will actually work even though a new JsRender instance will be loaded. But you will still probably not want to reload if you have registered named templates, since those registrations will be removed.

Update: The new version (65) changes behavior on reloading - and does not remove registered named templates. So that plus the new caching approach will both help your scenarios...

BorisMoore commented 9 years ago

This has been fixed in commit 65