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

Failing test for symlinks #9

Merged
merged 1 commit into from
Jan 9, 2015
Merged

Failing test for symlinks #9

merged 1 commit into from
Jan 9, 2015

Conversation

adgad
Copy link
Contributor

@adgad adgad commented Dec 24, 2014

/cc @matthew-andrews

Leaving this here as a failing test for following symlinks in partials dir. It's fixed in my fork of express-handlebars, which I've put in a PR for:
"express-handlebars": "adgad/express-handlebars#follow-symlinks",

https://github.com/ericf/express-handlebars/pull/98/files

@matthew-andrews
Copy link
Contributor

oh wow. really nice work.

@matthew-andrews
Copy link
Contributor

this is exactly the approach I strongly believe in — identify the root cause, fix it there… contribute back to open source, promote better understanding the internals of the tools we use (eg. I didn't know bower link used symlinks — I guess it's obvious now but I never thought about it… just assumed it was “Magic”), regression test, and a scalpel-like fix, really really nice work.

@matthew-andrews
Copy link
Contributor

If you like we can switch to using your branch's express-handlebars in ft-next-express. I guess we will see how long it takes to get the proper fixed merged in upstream?

@adgad
Copy link
Contributor Author

adgad commented Dec 24, 2014

Yeah let's see where it's at in the new year, if not then we can use the
fork

On 24 December 2014 at 11:01, Matt Andrews [email protected] wrote:

If you like we can switch to using your branch's express-handlebars in
ft-next-express. I guess we will see how long it takes to get the proper
fixed merged in upstream?


Reply to this email directly or view it on GitHub
#9 (comment)
.


This email was sent by a company owned by Pearson plc, registered office at
80 Strand, London WC2R 0RL. Registered in England and Wales with company
number 53723.

@adgad
Copy link
Contributor Author

adgad commented Jan 8, 2015

Okay, so the node-glob PR around following symlinks seems to have stalled. Given that the bower link workflow is quite important for us, I propose we operate off a fork of express-handlebars for now, with one of the following hacky solutions:

https://github.com/adgad/express-handlebars/compare/hacky-symlink-fix?expand=1
(I don't know enough about globs to know why that works, but it does)

OR rolling back node-glob to the v3.x branch (which had different symlink behaviour) as suggested here isaacs/node-glob#134.

Thoughts?

@matthew-andrews
Copy link
Contributor

Actually the hacky solution makes sense based on isaacs comment

The solution is not "follow symlinked dirs" as suggested in the OP here. The correct behavior is "stop following symlinked dirs as part of a globstar match, but still resolve their contents against subsequent parts of pattern".

Personally, though, I don't think it's hacky enough #14

@matthew-andrews matthew-andrews merged commit 1c17a23 into master Jan 9, 2015
@wheresrhys wheresrhys deleted the symlinks branch June 21, 2016 15:54
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.

2 participants