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 package's node + twigjs versions; update travis's node versions; bump #32

Merged
merged 7 commits into from
Jun 27, 2017

Conversation

olets
Copy link
Collaborator

@olets olets commented Nov 22, 2016

Updates to the latest twig.js


This PR has gotten bigger. Updated description:

The main purpose of this PR is to use the latest 1.x.y version of twig.js

gulp-twig's node requirements have fallen far enough behind twig.js's that updating to the latest twig.js makes gulp-twig's tests fail. So this PR also updates node in package.json and .travis.yml.

Upping the node version dependency should be considered a breaking change, so I've bumped to v1. This has the perk of fitting nicely with twig.js's versioning. I propose that gulp-twig's major version should always be the same as the major version of the twig.js dependency.

@olets olets changed the title twigjs 0.10 -> 0.10.1 update twigjs Feb 14, 2017
@ryuran
Copy link

ryuran commented Mar 2, 2017

Twig.js v1.10.4 is released
#32 and #34 are exactly for the same thing

@olets
Copy link
Collaborator Author

olets commented Mar 4, 2017

Sure is! Added 1 to the version specifically so that gulp-twig and anything else that uses twigjs as a dependency can specify semver ranges (which npm doesn't support for v<1.0.0).

Note gulp-twig's tests now fail (#35) but twigjs is passing so this should be safe to merge and use.

@ryuran
Copy link

ryuran commented Mar 5, 2017

We should change travis config (https://github.com/zimmen/gulp-twig/blob/master/.travis.yml) for recent node/npm.
And change node and npm minimal version in package.json.
We must up gulp-twig version to indicate new requirement.

@olets olets requested review from bezoerb and webrgp June 26, 2017 14:41
@olets olets changed the title update twigjs update package's node + twigjs; update travis's node Jun 26, 2017
@olets olets changed the title update package's node + twigjs; update travis's node update package's node + twigjs versions; update travis's node versions Jun 26, 2017
@olets
Copy link
Collaborator Author

olets commented Jun 26, 2017

@ryuran good catch thanks

@olets olets changed the title update package's node + twigjs versions; update travis's node versions update package's node + twigjs versions; update travis's node versions; bump Jun 26, 2017
package.json Outdated
@@ -23,7 +23,7 @@
"test": "mocha -R spec"
},
"engines": {
"node": ">= 0.9.0"
"node": "*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not change this to >=4.0 according Travis & Readme?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lifted this uncritically from twig.js. >=4.0 makes sense to me.

- "0.10"
- "0.11"
- 4
- 6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would also add 8

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

None of the dependencies test 8. twig.js tests 4 and 6. Since Twig is what we care about it makes sense to match its tests

Copy link
Collaborator

Choose a reason for hiding this comment

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

good point :)

@olets olets force-pushed the update-dependencies branch from 5f5c12a to f64df15 Compare June 27, 2017 03:28
@bezoerb
Copy link
Collaborator

bezoerb commented Jun 27, 2017

So let's merge this :)

@bezoerb bezoerb merged commit 8aea9c2 into simon-dt:master Jun 27, 2017
@bezoerb
Copy link
Collaborator

bezoerb commented Jun 27, 2017

@olets, @Dotmagic, @webrgp: i think we should consider #18, #19 and #21 before v1.0.0 release. What do you think?

@olets
Copy link
Collaborator Author

olets commented Jun 28, 2017

I think go with 1.0.0 now. For me, dependencies like this should follow semver - so bumping from 0.x to 1.0 is saying "there was a breaking change" not "we've ironed out the bugs." The upgraded node requirement is a breaking change.

If we do use a lower version, we'll have to remember to downgrade the version in package.json from the bump in f64df15

@olets olets deleted the update-dependencies branch September 7, 2017 14:47
@olets olets mentioned this pull request Sep 7, 2017
@olets olets mentioned this pull request Sep 11, 2017
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