pugjs / pug

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

variable scope with "include" is not good enough #2487

Open antpaw opened 8 years ago

antpaw commented 8 years ago

I just run into a problem related to includes and variable scope from the parent partial.

//- template1.pug
for myScopedVar in [1, 2, 3] 
  include low_level_template
//- low_level_template.pug
p= myScopedVar //=> "<p>undefined</p>" with pug-loader on webpack
p= myScopedVar //=> "<p>1</p>" with pug on node.js

At first it looks like its a pug-loader bug, it simply ignores this language feature, but it does it for a very good reason. In Node.js it's not a big deal to "inline" a include and get the outer template's scope for free. But on client-side it's more tricky because you want to optimize your includes into require statements, so the file size is as small as possible.

The best fix for this would be if pug had two different syntaxes for include that let programmers decide to inline or to require the template.

include ./my_partial // has parent's template scope, stays they way it is.
require ./my_partial // loses parent's template scope

Or a syntax that provides a way to define variables that should be passed down to the include along with already set variables. (I think that's the better options)

include ./my_partial, {myVar: true, myOtherVar: [1, 2, 3]}

I notices something about a "plugin" system in pug, but couldn't find any documentation about it. Is it technically possible to write a plugin that would change/extend pug to the first or second syntax option?

I'm sorry if a discussion about this problem already exists, I couldn't find any.

TimothyGu commented 8 years ago

In the context of Node.js, unless we registers Pug as a require handler, having a require keyword with a separate scope does not make much sense (either logically or implementation-wise), so I'm against making it part of Pug core.

Plugins are probably the best way to add a require keyword for Webpack and other browser packers.

The docs for Pug are indeed not complete yet. In fact, plugins are currently the biggest missing part. Basically, it works like this:

const plugin1 = {
  postParse: (ast) => {
    return transform(ast, options);
  },
  read: (path) => {
    return fs.readFileSync(path, 'utf8');
  },
  // ...
};

pug.compile(src, {
  plugins: [
    plugin1,
    plugin2
  ]
});

Where hooks are applied can be seen in https://github.com/pugjs/pug/blob/master/lib/index.js (grep for plugins).

I've also written a few other plugins, like one that allows quasi-dynamic inclusion by replacing the values of path tokens with interpolated values to allow including different files for different languages.

In case you are interested, a require plugin would require (no pun intended) a lexer plugin that scans the stream for require keywords and output a set of require-path tokens. In the parser, translate these sets require-path tokens to a Code AST node much like what pug-loader already does.

TimothyGu commented 8 years ago

On a second thought, such a syntax may be interesting in Node.js as well:

//- main.pug
import { nav, button } from mixins.pug

doctype
html
  head
    title Page
  body
    +nav
    +button('test')
//- mixins.pug
mixin navGroup
  ul

export mixin nav
  nav.nav
    +navGroup
      li Entry

export mixin button(title)
  button.btn= title

That'd have to be reserved for the next major release though.

ForbesLindesay commented 8 years ago

import/export syntax for mixins with proper scope rules is probably the right approach here for supporting this in the language. Regarding having to reserve the keywords, my plan would be to do @import and @export and move towards all new keywords being @ prefixed (and maybe long term all existing keywords as well). This will allow us to add keywords with much less concern for backwards compatibility.

Regarding pug-loader's odd behaviour, I'd regard this as a bug. It is almost certainly counter-productive in reality to split included pug code into separate requireed files, since the overhead of all the extra function scopes created probably outweighs the small benefit of a couple of strings not being duplicated (gzip does a lot to help here). Without benchmarking it would be impossible to know for sure, but breaking the semantics of pug seems very wrong here.

TimothyGu commented 8 years ago

breaking the semantics of pug seems very wrong here

Agreed, although again I was just following the semantics set up by jade-loader, which was written by the author of Webpack (@sokra).

The prefixing of keywords definitely needs a lot more thought. True, it can be less confusing, but it's also more typing.

CharlesGouldmann commented 4 years ago

Any progress regarding this bug? I've run into the same issue where I am unable to access a variable from the parent in my included template.