pugjs / pug

Pug – robust, elegant, feature rich template engine for Node.js
https://pugjs.org
21.67k stars 1.95k forks source link

All global variables are accessible now making options.globals pointless #1773

Open mudassir0909 opened 9 years ago

mudassir0909 commented 9 years ago

Say this is my jade template

// Jade Template
div= location
// my locals
{}

I'd expect an empty div tag to be rendered but instead it renders with the current url inside div tag.

Fiddle

The reason is due to the following code here

var inputVars = vars.map(function (v) {
    return JSON.stringify(v) + ' in ' + local + '?' +
      local + '.' + v + ':' +
      'typeof ' + v + '!=="undefined"?' + v + ':undefined'
});

compiles to

var location = "location" in locals_for_with ? 
                          locals_for_with.location :
                          ( typeof location !== "undefined" ? location : undefined );

This code basically makes all the variables defined under window available in the jade template.

ForbesLindesay commented 9 years ago

The globals option is still somewhat useful for people who are using static analysis. e.g. browserify looks for calls to require('string literal') but will miss them if we use the runtime lookup.

mudassir0909 commented 9 years ago

But I'm worried about window variables being available in Jade, I've written code assuming only the ones defined inside globals array will be available. I've some column names on the database like location which are being used directly in templates. If the column value is null then the url is being rendered.

Also, by the way, was this change intentional ?

ForbesLindesay commented 9 years ago

Yes, it vastly reduced the number of issues along the lines of "such and such a global isn't accessible" or "such and such a variable doesn't work as a local". As long as you pass a location, even if the location you pass is undefined, your template should work fine.

We could add a locals option to mimic the globals option. We could also consider allowing something like globals: false to indicate that there are no globals, effectively forcing templates to only access locals.

mudassir0909 commented 9 years ago

Basically a configuration which just returns undefined if the attribute is not present inside the object that is passed as first argument to addWith

The problem is due to the with module,

$ node
> addWith = require('with')
> eval(addWith('{a: 123}', 'console.log(a)'))
123 #which is perfectly fine
> eval(addWith('{a: 123}', 'console.log(require)'))
{ [Function: require] # It should have printed undefined instead of printing global.require
  resolve: [Function],
  main: undefined,
 ....
}

I guess changing the function at https://github.com/ForbesLindesay/with/blob/master/index.js#L43 to the following would solve the problem

  var inputVars = vars.map(function (v) {
    return local + '[' + JSON.stringify(v) + ']';
  })

Let me know if this makes sense so that I can generate a PR for this against https://github.com/ForbesLindesay/with

TimothyGu commented 9 years ago

I'm reassigning this to 3.0.0 since 2.0.0 is about to be published and this is not top-priority.

chaaz commented 8 years ago

This bit me when I had an optional property on my locals that happened to match the id of a DOM element. The HTML5 spec here: https://html.spec.whatwg.org/multipage/browsers.html#named-access-on-the-window-object says that such DOM elements can be accessed directly via window[<id>], which means that Jade can see such things in globals. :( Imagine my surprise when I figured out Jade was trying to use an element instead of undefined.

I can work around for now, but I'd be in favor of a globals: false or something like that.

niftylettuce commented 6 years ago

This exact problem is causing a conflict when my templates are rendered. If you have an anchor with the same name as a local variable then your client-side rendered templates will be corrupt and will not work. I've provided a brief example below, cc @ForbesLindesay this should be marked as a bug.

If you have a anchor on your page like <div id="someArray">Hello</div> and you pass in your locals object a locals.someArray: [ 'item1', 'item2' ]' to be rendered in your Pug template, then the value of locals.someArray will be window.someArray, which is just a reference to the DOM element for the anchor:

if someArray.length > 0
  ul
    each item in someArray
      li= item
niftylettuce commented 6 years ago

Is there any way to use locals first and fallback to globals somehow? Doesn't seem globals option is available yet and the code for with has change drastically since the code that was shared and mentioned above.