Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update glob and follow symlinks when looking for partials #98

Closed
wants to merge 1 commit into from

Conversation

adgad
Copy link

@adgad adgad commented Dec 24, 2014

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.

@@ -242,7 +242,7 @@ ExpressHandlebars.prototype._getDir = function (dirPath, options) {
// Optimistically cache dir promise to reduce file system I/O, but remove
// from cache if there was a problem.
dir = cache[dirPath] = new Promise(function (resolve, reject) {
glob(pattern, {cwd: dirPath}, function (err, dir) {
glob(pattern, {cwd: dirPath, symlinks: true}, function (err, dir) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at Glob's options it says the following for symlinks:

A cache of known symbolic links. You may pass in a previously generated symlinks object to save lstat calls when resolving ** matches.

So I'm not sure how passing a value of true would have any affect on Glob and make it follow symlinks. Can you explain more how this changes the behavior of Glob?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bah, have a feeling this might just be a horrid hack - was just trying
stuff out and it happened to work in my case. Will take a proper look in
the new year - but maybe this is more like what we'd want:
isaacs/node-glob#142

On Wed, Dec 24, 2014 at 5:48 PM, Eric Ferraiuolo [email protected]
wrote:

In lib/express-handlebars.js
#98 (diff)
:

@@ -242,7 +242,7 @@ ExpressHandlebars.prototype._getDir = function (dirPath, options) {
// Optimistically cache dir promise to reduce file system I/O, but remove
// from cache if there was a problem.
dir = cache[dirPath] = new Promise(function (resolve, reject) {

  •    glob(pattern, {cwd: dirPath}, function (err, dir) {
    
  •    glob(pattern, {cwd: dirPath, symlinks: true}, function (err, dir) {
    

Looking at Glob's options it says:

A cache of known symbolic links. You may pass in a previously generated
symlinks object to save lstat calls when resolving ** matches.

So I'm not sure how passing a value of true would have any affect on Glob
and make it follow symlinks. Can you explain more how this changes the
behavior of Glob?


Reply to this email directly or view it on GitHub
https://github.com/ericf/express-handlebars/pull/98/files#r22260888.

@pushred
Copy link

pushred commented Jan 26, 2015

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
Copy link
Author

adgad commented Jan 26, 2015

adgad@803129e (or hacked into our code here Financial-Times/n-express@0c78b6f)

This is a better workaround we're using for now. Adding the "subsequent part of a pattern" as mentioned by @isaacs here isaacs/node-glob#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
Copy link

pushred commented Jan 27, 2015

Works for me too, thanks for sharing an update!

@ericf
Copy link
Owner

ericf commented Feb 12, 2015

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
Copy link
Author

adgad commented Feb 13, 2015

Right, there's not been any movement on the node-glob issue, so I've created another PR with the better //* fix. #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
Copy link
Owner

ericf commented Feb 13, 2015

@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: broccolijs/broccoli-kitchen-sink-helpers#28, and once isaacs/node-glob#139 is resolved we can upgrade and go with the {follow: true} option.

@adgad
Copy link
Author

adgad commented Feb 13, 2015

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 ericf closed this in a91ae6b Feb 17, 2015
@ericf
Copy link
Owner

ericf commented Feb 17, 2015

This fix is in v1.2.1.

ericf added a commit that referenced this pull request Mar 6, 2015
This dependency can be updated now that this issue is resolved:
isaacs/node-glob#139

See #98, #106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants