ericf / express-handlebars

A Handlebars view engine for Express which doesn't suck.
BSD 3-Clause "New" or "Revised" License
2.31k stars 383 forks source link

Update glob and follow symlinks when looking for partials #98

Closed adgad closed 9 years ago

adgad commented 9 years ago

Currently, partials don't follow symbolic links.

A common use case (and one we're using) is registering './bower_components' as a partials dir - which works fine - but then trying to use bower link (which creates a symbolic link to a local copy of the component repo) to develop locally between components.

pushred commented 9 years ago

FWIW, this is also working for me. In my case I'm using npm link to develop partials within a library module that I'm using in a project with the changes in #81. They're just not registered however:

Error: The partial library/example could not be found
    at new Error (<anonymous>)
    at Error.Handlebars.Exception (/Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/handlebars/lib/handlebars/utils.js:10:41)
    at Object.Handlebars.VM.invokePartial (/Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/handlebars/lib/handlebars/runtime.js:88:13)
    at Object.eval (eval at <anonymous> (/Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/handlebars/lib/handlebars/compiler/compiler.js:579:23), <anonymous>:161:17)
    at /Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/handlebars/lib/handlebars/runtime.js:38:33
    at /Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/handlebars/lib/handlebars/compiler/compiler.js:1294:21
    at ExpressHandlebars._renderTemplate (/Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/express-handlebars/lib/express-handlebars.js:310:12)
    at ExpressHandlebars.<anonymous> (/Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/express-handlebars/lib/express-handlebars.js:184:21)
    at /Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/express-handlebars/node_modules/promise/lib/core.js:33:15
    at flush (/Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/express-handlebars/node_modules/promise/node_modules/asap/asap.js:27:13)
    at /Users/eric/dev/sites/example.com/node_modules/solidus/node_modules/continuation-local-storage/node_modules/async-listener/glue.js:188:31
    at process._tickCallback (node.js:442:13)

If I take a look at the paths returned by glob, using symlinks: true my library partials are included whereas without it they're excluded. Since they're never found, they're not registered.

adgad commented 9 years ago

https://github.com/adgad/express-handlebars/commit/803129ed9b325640cc527aefdb5096dda7661355 (or hacked into our code here https://github.com/Financial-Times/next-express/commit/0c78b6f94249311dfe50dfcff7bf36f5fc491223)

This is a better workaround we're using for now. Adding the "subsequent part of a pattern" as mentioned by @isaacs here https://github.com/isaacs/node-glob/issues/134 seems to match our symlinked partials (and makes a bit more sense).

Happy to put in a PR if nobody sees a problem with it - however The proposed "options.follow" here still looks like it's a more self-documenting way of achieving the same thing.

pushred commented 9 years ago

Works for me too, thanks for sharing an update!

ericf commented 9 years ago

Where are we at with this one? I'm planning to release v2.0 soon which uses Handlebars 3.0 by default (#105), and I'd like to get this into that release.

adgad commented 9 years ago

Right, there's not been any movement on the node-glob issue, so I've created another PR with the better //* fix. https://github.com/ericf/express-handlebars/pull/106

Doesn't seem to have any negative performance impact for me (albeit with a very unscientific check!), so I'd imagine it's good to go

ericf commented 9 years ago

@adgad would a better option be to simply lock down the glob dep to the version before this change was added? That way symlinks will be followed.

This is what they're doing for Broccoli: https://github.com/broccolijs/broccoli-kitchen-sink-helpers/pull/28, and once https://github.com/isaacs/node-glob/issues/139 is resolved we can upgrade and go with the {follow: true} option.

adgad commented 9 years ago

Yup that could work also. Just tried it now, last working version for me (v4.0.6) is still on v4.* so hopefully won'y be a breaking change if/when the {follow: true} option arrives.

ericf commented 9 years ago

This fix is in v1.2.1.