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

Added Support for Twing #327

Merged
merged 13 commits into from
Nov 2, 2019
Merged

Added Support for Twing #327

merged 13 commits into from
Nov 2, 2019

Conversation

noel-schenk
Copy link
Contributor

Twing is a template engine which supports 100% of the syntax defined by Twig.
https://github.com/ericmorand/twing

@noel-schenk
Copy link
Contributor Author

noel-schenk commented Sep 19, 2019

Twing starts with node version 6.0.0 so it's excluded from the node tests under 6.0.0.
If someone were to use a node version older than 6.0.0 it would throw an error message explaining that it has to be upgraded.

@doowb
Copy link
Collaborator

doowb commented Sep 19, 2019

Thanks for the PR. I'll review closer this weekend, but for now, I don't think bringing in semver for this check is the right thing to do.

I'm pretty sure there are some other template engines that don't support older versions of Node.js and are handled in the tests (I'll look into this later). I think doing a simple check in the tests would be sufficient.

I'd like to get this library updated to drop older versions of Node.js anyway so it won't be an issue then.

@ericmorand
Copy link
Contributor

I think dropping node 4 support should be safe. Don't know what the usage stats are exactly but I'm very enclined to believe that node 4 is barely used anymore.

@DerekRoth
Copy link

Any update on this ? Twing is now in the official list of ExpressJS "out of the box" supported template engines but i believe it depends on this PR being merged ? See https://expressjs.com/en/resources/template-engines.html

@noel-schenk
Copy link
Contributor Author

@doowb is there any way we can circumvent the node 4 tests?

@doowb
Copy link
Collaborator

doowb commented Oct 30, 2019

@NoelElias would you move the semver dependency to devDependencies and only use it in the tests (just like you already did).

Since consolidate itself doesn't require in the engine until it's used this should be fine. We can see that the Node.js 4 tests pass, which I'm fine with.

lib/consolidate.js Outdated Show resolved Hide resolved
@noel-schenk
Copy link
Contributor Author

@doowb it should be ready to merge

Copy link
Collaborator

@doowb doowb left a comment

Choose a reason for hiding this comment

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

@NoelElias thanks for the changes, but I meant that we shouldn't use semver in the "production" code, just in the tests. I made a couple of comments in line. Then I think it will be good to merge.

lib/consolidate.js Outdated Show resolved Hide resolved
lib/consolidate.js Outdated Show resolved Hide resolved
@doowb
Copy link
Collaborator

doowb commented Nov 2, 2019

Thanks!

@doowb doowb merged commit 31cfe78 into tj:master Nov 2, 2019
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.

4 participants