batiste / pug-vdom

PUG template to HyperScript Virtual DOM
MIT License
18 stars 6 forks source link

Calling render function inside another render function makes local variable disappear. #42

Closed albb0920 closed 4 years ago

albb0920 commented 4 years ago

In pug-vdom, locals are exposed as properties on global object during the render. Only when an existing property of the same name exists on the global object does pug-vdom put it in the local scope. https://github.com/batiste/pug-vdom/blob/master/pug-vdom.js#L63

Currently, I'm having trouble when calling render function of a pug template inside another pug template. This is due to the inner pug render function calls deleteExposedLocals() when it's done, it clears a variable that the outer pug render function uses, due to both templates uses a local variable of the same name.

I think there's two way to fix this:

I'm wondering what's the rationals behind exposing locals as globals, and what would be the preferred fix?

Just for reference, the official implementation compiles

- x(y)

as

function template(locals) {
    var pug_html = "",
        pug_mixins = {},
        pug_interp;
    var pug_debug_filename, pug_debug_line;
    try {
        ;
        var locals_for_with = (locals || {});
        (function(x, y) {
            ;
            pug_debug_line = 1;
            x(y)
        }.call(this, "x" in locals_for_with ? locals_for_with.x : typeof x !== "undefined" ? x : undefined, "y" in locals_for_with ? locals_for_with.y : typeof y !== "undefined" ? y : undefined));
    } catch (err) {
        pug.rethrow(err, pug_debug_filename, pug_debug_line);
    };
    return pug_html;
}

Thanks!

gryphonmyers commented 4 years ago

It was done as a performance optimization - in my experience the eval here https://github.com/batiste/pug-vdom/blob/master/pug-vdom.js#L66 becomes expensive when the locals object is very large. Instead of evaluating all locals, it will instead try to set them as getters on the global scope, which is comparatively cheap.

Good idea looking at the pug source :) Looks like they pass all the locals into a closure. That's not bad - it might still be more expensive than the lazy getters but would likely be cheaper than the individual evals.

In any case, your use case was not something I had accounted for so let me write some unit tests covering it and see if I can come up with a snappy solution.

albb0920 commented 4 years ago

Thanks you for your detailed response.

pug relies on the with package for this feature. Using this package will make pug-vdom behave more like pug, but will also be a breaking change. (eg. locals cannot be accessed in eval())

Since I have to make this work in my project, I think I can send in a PR as a baseline for this. Thanks!

gryphonmyers commented 4 years ago

Question: I'm attempting to recreate the conditions you've described above but I'm having difficulty getting my unit test to fail. Does this look correct?

innerTemplate:

div
    .inner-text= myText

outer template

div
    .text= myText
    | !{innerTemplate({ myText: myInnerText }, h)}

with { myText: 'foo', myInnerText: 'bar', innerTemplate }

I get

<div><div class="text">foo</div><div><div class="inner-text">bar</div></div></div>

as output. As far as I can tell that looks correct. Am I not modeling this correctly?

gryphonmyers commented 4 years ago

Thanks you for your detailed response.

pug relies on the with package for this feature. Using this package will make pug-vdom behave more like pug, but will also be a breaking change. (eg. locals cannot be accessed in eval())

Since I have to make this work in my project, I think I can send in a PR as a baseline for this. Thanks!

If you want to take a stab, go for it!

albb0920 commented 4 years ago

Here's a snippet to demo this:

var pugVDOM = require('pug-vdom')
require('pug-vdom/runtime')

var inner = pugVDOM.generateTemplateFunction(`= x`)

var outer = pugVDOM.generateTemplateFunction(`
= inner({x: x})
= x + "2"
`)

console.log(outer({x: "a", inner: inner}))

causes

undefined:12
  n0Child = n0Child.concat(x + "2")
                           ^

ReferenceError: x is not defined
    at render (eval at generateTemplateFunction (/home/albb0920/test/x/node_modules/pug-vdom/pug-vdom.js:25:12), <anonymous>:12:28)
    at Object.<anonymous> (/home/albb0920/test/x/demo.js:11:13)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3)

Thanks!

gryphonmyers commented 4 years ago

OK I have added a failing test using your example: https://github.com/batiste/pug-vdom/tree/ft-nested-template-bug. I will probably not get a chance to look at this any further today