939214 - template() function cannot be called inside a function

Status: RESOLVED WONTFIX

(developer.mozilla.org Graveyard :: KumaScript, defect)

Description

[Jeremie Patonnier :Jeremie]

In some case when calling the template function inside a function, kumascript think it is undefined.

Here is a buggy script:
https://developer.mozilla.org/en-US/docs/Template:APIRefBugTestCase

Here is the error:
https://developer.mozilla.org/en-US/docs/User:Jeremie/BluetoothManager

Comment 1

[Jean-Yves Perrier [:teoli]]

Shouldn't we use template("macroName", [param1, param2, ...]) in KumaScript to call another macro?

Comment 2

[Les Orchard [:lorchard]]

I think this is actually a known issue we work around in a lot of other macro scripts too. 

Not sure exactly what's going on with variable scope in the EJS engine KumaScript uses, but lots of "built-ins" like template() are not available within functions. I've never been able to find a fix for this.

Comment 3

[Les Orchard [:lorchard]]

For what it's worth, this is the engine that processes macro scripts in KumaScript:

https://github.com/visionmedia/ejs

Comment 4

[Eric Shepherd [:sheppy]]

This is a fairly significant problem in that we usually can work around it but there have been some time-sensitive situations in which we've been unable to get things to work correctly because of this.

Comment 5

[Les Orchard [:lorchard]]

The core problem is a node.js / V8 platform bug. I mentioned this on IRC many months ago, but never updated this bug. 

The template engine uses the `with` keyword to expose variables to templates. In V8, these variables do not get exposed to functions defined within a `with` block. Here's a demo of the issue:

http://jsfiddle.net/qc1g1x6d/4/

Open that in Firefox, and you should see "foo = bar; baz = quux"

Open it in Chrome, and you'll see "foo is undefined!; baz = quux"

Since node.js is based on Chrome's V8, it also suffers from this issue. And, that suggests this bug won't get resolved short of a bugfix in V8 itself - or some kind of rewrite of the template engine. Neither of those sound trivial.

It's clumsy, but there is a possible workaround in the second half of the jsfiddle:

    (function (baz) {

        function thing2 () {
            document.body.innerHTML += 
                ('undefined' == typeof baz) ?
                    'baz is undefined!' :
                    'baz = ' + baz;
        }
        thing2();

    }(baz));

That's an immediately-invoked function expression that ensures a closure exists containing the variable `baz`, so that it's made available to the function `thing2`. Ideally, that's what `with` should do, but doesn't in V8.

Comment 6

[Les Orchard [:lorchard]]

Also FWIW, I just filed this issue on V8: https://code.google.com/p/v8/issues/detail?id=3676

Comment 7

[Sebastian Zartner [:sebo]]

It looks like the it can be defined through setting it outside the function:

<%
function test() {
    tpl("non-standard_inline");
}

var tpl = template;
%><%- test() %>

This allows to call the function from within another function, though the call still fails. This time with the following error:

Cannot call method 'push' of undefined

referring to /kuma/kumascript/lib/kumascript/api.js:339:20

See the example here:
https://developer.allizom.org/en-US/docs/User%3ASebastianz/Call_to_template%28%29_from_within_function
https://developer.allizom.org/en-US/docs/Template%3Afunction_test

So could that be fixed somehow?

Also, do we already use version 2 of ejs[1]? Could it be that this version fixes the problem?

Sebastian

[1] https://github.com/mde/ejs

Comment 8

[Les Orchard [:lorchard]]

(In reply to Sebastian Zartner [:sebo] from comment #7)
> It looks like the it can be defined through setting it outside the function:
> ...
> This allows to call the function from within another function, though the
> call still fails. This time with the following error:
> 
> Cannot call method 'push' of undefined
> 
> referring to /kuma/kumascript/lib/kumascript/api.js:339:20

The template() function is part of an API object defined in that JS module:

https://github.com/mozilla/kumascript/blob/master/lib/kumascript/api.js

If you assign it to a variable and call it, that breaks its reference to `this` and causes that error. You'd have do do something like `var tpl = template.bind(api)`

> Also, do we already use version 2 of ejs[1]? Could it be that this version fixes the problem?

This is a problem in V8. The ejs module (even v2) still uses the `with` keyword to inject psuedo-global variables into the scope of the code executed as a template, and that's what's broken at the V8 / node.js level.

FWIW, I haven't worked on MDN in almost a year, so my info is most likely out of date. You should probably needinfo someone from the core team or poke around on #mdn in IRC

Comment 9

[Sebastian Zartner [:sebo]]

(In reply to Les Orchard [:lorchard] from comment #8)
> (In reply to Sebastian Zartner [:sebo] from comment #7)
> > It looks like the it can be defined through setting it outside the function:
> > ...
> > This allows to call the function from within another function, though the
> > call still fails. This time with the following error:
> > 
> > Cannot call method 'push' of undefined
> > 
> > referring to /kuma/kumascript/lib/kumascript/api.js:339:20
> 
> The template() function is part of an API object defined in that JS module:
> 
> https://github.com/mozilla/kumascript/blob/master/lib/kumascript/api.js
> 
> If you assign it to a variable and call it, that breaks its reference to
> `this` and causes that error. You'd have do do something like `var tpl =
> template.bind(api)`

I've already tried that, though as far as I saw this 'api' parameter is not exposed.

> > Also, do we already use version 2 of ejs[1]? Could it be that this version fixes the problem?
> 
> This is a problem in V8. The ejs module (even v2) still uses the `with`
> keyword to inject psuedo-global variables into the scope of the code
> executed as a template

I didn't try it out myself, but it seems in version 2 of EJS you can turn off the usage of `with` by setting `{_with: false}`:
https://github.com/mde/ejs#user-content-options

> FWIW, I haven't worked on MDN in almost a year, so my info is most likely
> out of date. You should probably needinfo someone from the core team or poke
> around on #mdn in IRC

Ok, so Luke, regarding my sentence above, would it make sense to update EJS to version 2?

Sebastian

Comment 10

[Eric Shepherd [:sheppy]]

If that _with option could potentially resolve things, that would be a huge deal. Is there a way to test the theory? I have no idea how big a project it would be to swap in ejs 2.

Comment 11

[Sebastian Zartner [:sebo]]

AFAICS we'd at least have to prepend all usages of global functions by 'locals.'.
So if that's right, I believe doing that makes more sense after tightening up and reducing the number of templates. (There was the intent to do so, right? Can someone point me to where this is tracked?)

Sebastian

Comment 12

[Luke Crouch [:groovecoder]]

Catching up on needinfo? and looping in :davidwalsh.

Moving KumaScript to EJS2 is a big (1-2 months) and scary (lots of edge cases could break) project to me; especially if we change _with: false and have to audit and update all our code to use locals.<global function>

+1 :sebo - tightening our KumaScript usage and operations should happen first. Then there are fewer edge cases, and better quality controls wrapped around the engine we'd be updating.

Comment 13

[Eric Shepherd [:sheppy]]

(In reply to Luke Crouch [:groovecoder] from comment #12)
> Catching up on needinfo? and looping in :davidwalsh.
> 
> Moving KumaScript to EJS2 is a big (1-2 months) and scary (lots of edge
> cases could break) project to me; especially if we change _with: false and
> have to audit and update all our code to use locals.<global function>
> 
> +1 :sebo - tightening our KumaScript usage and operations should happen
> first. Then there are fewer edge cases, and better quality controls wrapped
> around the engine we'd be updating.

I agree in principle, but it sure would be easier to clean up our KumaScript code (including template reduction by merging nearly-identical templates and using arguments to differentiate) if we weren't somewhat hobbled by this issue, too.

But I totally recognize that this is scary work. On the other hand, modernization of the EJS code probably has to happen someday anyway, so keeping it on the stack of things to do when we can would be wise, I think.

Comment 14

[Sebastian Zartner [:sebo]]

(In reply to Sebastian Zartner [:sebo] from comment #11)
> ... tightening up and reducing the number of templates. (There was the intent to do so, right? Can someone point me to where this is tracked?)

It seems my question from comment 11 was missed, so I'm asking again. Do we already have an issue tracking the template cleaning task?

Sebastian

Comment 15

[Eric Shepherd [:sheppy]]

(In reply to Sebastian Zartner [:sebo] from comment #14)

> It seems my question from comment 11 was missed, so I'm asking again. Do we
> already have an issue tracking the template cleaning task?

I don't know the bug number. I assume that Florian or Teoli can share that.

Comment 16

[Florian Scholz (Open Web Docs)]

(In reply to Sebastian Zartner [:sebo] from comment #14)
> It seems my question from comment 11 was missed, so I'm asking again. Do we
> already have an issue tracking the template cleaning task?

We are in the process of discussing a project for this. Once we agreed on a plan and the details of that project, bugs will be filed.

I just started this thread for that: https://groups.google.com/forum/#!topic/mozilla.dev.mdc/rPT8vgZdJBk
I welcome your thoughts :)

Comment 17

[BMO Automation]

MDN Web Docs' bug reporting has now moved to GitHub. From now on, please file content bugs at https://github.com/mdn/sprints/issues/ and platform bugs at https://github.com/mdn/kuma/issues/.