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

Save nunjuck env reference in express app settings #829

Merged
merged 2 commits into from
Aug 31, 2016
Merged

Save nunjuck env reference in express app settings #829

merged 2 commits into from
Aug 31, 2016

Conversation

lukechilds
Copy link
Contributor

Resolves #828

@@ -286,6 +286,7 @@ var Environment = Obj.extend({
};

app.set('view', NunjucksView);
app.set('nunjuckEnv', this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Its nunjucks, with the s, so this should be nunjucksEnv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted 👍

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

Can you add a test for this?

@lukechilds
Copy link
Contributor Author

I've added what I think should be a passing test but I'm not quite sure how to run the express tests. npm test doesn't seem to do anything with that file and I can't find anything documenting it. Any ideas?

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

Huh, looks like the express tests aren't running. Odd. I'll create a separate issue for that. Test looks fine, anyway, I'll go with it for now.

@carljm carljm merged commit 0c3a03b into mozilla:master Aug 31, 2016
carljm pushed a commit that referenced this pull request Aug 31, 2016
* Save nunjuck env reference in express app settings

* Add test for express nunjucks env reference
@lukechilds
Copy link
Contributor Author

@carljm Yeah, it definitely works, I've manually copied this version into node_modules and it's working great.

Don't mean to rush you, sure you're busy, but just wondering if you had any plans to release this on npm soon? I'm currently working on a time sensitive project and it would be awesome to be able to get rid of my current workaround from the issue I posted.

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

I'll try to make a release soon.

@lukechilds
Copy link
Contributor Author

@vecmezoni great to see you've stepped up to help maintain this project. I just noticed you've published a new build to npm. Is there any chance you could publish a version with this PR in?

@vecmezoni
Copy link
Collaborator

@lukechilds ok, tomorrow i'll check 2.x branch and publish 2.5.0 😉

@lukechilds
Copy link
Contributor Author

That would be amazing, thank you! 🍻

@vecmezoni
Copy link
Collaborator

@lukechilds can't wait till tomorrow, published 2.5.0 with this PR.

@lukechilds
Copy link
Contributor Author

@vecmezoni working perfectly, cheers.

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