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

Add ability to expose app and request locals as private variables #63

Merged
merged 1 commit into from
Jun 2, 2014

Conversation

scottgonzalez
Copy link
Contributor

Fixes #57

I went ahead and implemented this as hbs.provideLocals(). Let me know if you want a different API and I'll change it. I can also add a full example in the examples directory if you'd like.

I wrote a test, but it's being marked as a failure even though there's no visible difference.

@defunctzombie
Copy link
Collaborator

Is this private variable syntax new to handlebars? We have no support for it in hbs currently?

@scottgonzalez
Copy link
Contributor Author

This has already been discussed in #57. See #57 (comment)

@scottgonzalez
Copy link
Contributor Author

Is there anything else that needs attention/changing/anything other than picking a name for the new function before this can land?

@jas
Copy link

jas commented May 19, 2014

Thank you @scottgonzalez! Can't wait to use this.

@defunctzombie
Copy link
Collaborator

Code looks good otherwise. After thinking about the API, the only limiting factor I can think of with this approach over app.use() is the ability to limit which routes this is enabled for. But I think this is cleaner and will lead to fewer errors than the app.use() approach.

As for the name. Something to convey exposing as app and response locals as template variables would be good. Could even drop the "expose" or "provide" part and just go with "localsAsTemplateData" and document it appropriately.

@scottgonzalez
Copy link
Contributor Author

I renamed the method in ed3bd38, but I still have no idea why the test is failing since the actual and expected values are the same.

@jas
Copy link

jas commented May 27, 2014

@scottgonzalez The expected output should have empty lines after each sentence. This is because the {{#each}} precedes a newline (for each, \nText\n)


Alan has a kid named Jimmy.

Alan has a kid named Sally.

@scottgonzalez
Copy link
Contributor Author

Adding an empty line reduces the number of errors from 7 to 3, but doesn't cause the test to pass. If you're able to get a working test suite, can you please just make the change?

@jas
Copy link

jas commented May 30, 2014

@scottgonzalez Sure thing. I updated the fixture and made another PR since I couldn't add to this one: #65

@scottgonzalez
Copy link
Contributor Author

Thanks @jas. I'm not sure why, but making that same change locally didn't work for me (I tried several times, even starting from a clean state multiple times). Anyway, it's working for me now that I've pulled your branch. I'll push your commit into this PR.

As a tip for the future, you can send a PR against my fork for me to pull into my branch. It's a bit indirect, but a lot cleaner than a duplicative PR on the main repo.

Thanks again.

@jas
Copy link

jas commented May 30, 2014

As a tip for the future, you can send a PR against my fork for me to pull into my branch. It's a bit indirect, but a lot cleaner than a duplicative PR on the main repo.

@scottgonzalez Ah, right! I forgot about that. I'll close the other.

@defunctzombie
Copy link
Collaborator

Looks good. Please squash commits and I will merge.

@scottgonzalez
Copy link
Contributor Author

Squashed and rebased.

@defunctzombie defunctzombie merged commit 894b59c into pillarjs:master Jun 2, 2014
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.

Provide app and response locals as lexical data
4 participants