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

Drop support for node 0.8 #334

Closed
wants to merge 2 commits into from
Closed

Drop support for node 0.8 #334

wants to merge 2 commits into from

Conversation

c089
Copy link
Member

@c089 c089 commented Feb 27, 2014

No description provided.

@alexindigo
Copy link
Member

Any specific reasons to limit Rendr's install base?

@lo1tuma
Copy link
Member

lo1tuma commented Feb 27, 2014

@alexindigo yes see discussion in #332.

@c089: LGTM, what do you think, should we mention this in HISTORY.md?

@alexindigo
Copy link
Member

@lo1tuma Thanx for the link. Although it's not exactly clear what was the reason. If it's coveralls then coveralls itself supports 0.8.

  "engines": {
    "node": ">=0.8.6"
  },

Or did I miss something?

@lo1tuma
Copy link
Member

lo1tuma commented Feb 27, 2014

It is not coveralls, the reason is the outdated npm version which is
bundled in node 0.8. This npm version doesn't support the ^ in version
patterns (see the outdated diff discussion in #332) and peerDependencies
which was a problem in #246.

@lo1tuma https://github.com/lo1tuma Thanx for the link. Although it's
not exactly clear what was the reason. If it's coveralls then coverallsitself supports
0.8.

"engines": {
"node": ">=0.8.6"
},

Or did I miss something?

Reply to this email directly or view it on GitHubhttps://github.com//pull/334#issuecomment-36268189
.

@alexindigo
Copy link
Member

Sure, that's my point. We can live with old notation and still support current and previous stable version, I don't think it justified to exclude possible usage just on the basis of different notation.

Let's wait until 0.12 and then drop support for 0.8, so we'd still support current and previous stable version.

@lo1tuma
Copy link
Member

lo1tuma commented Feb 27, 2014

Nope, using rendr with node 0.8 and browserify will fail anyway as you can
see in #246. We should also drop 0.8 support for rendr-handlebars which
explicitly makes use of the peerDependency feature.

Sure, that's my point. We can live with old notation and still support
current and previous stable version, I don't think it justified to exclude
possible usage just on the basis of different notation.

Let's wait until 0.12 and then drop support for 0.8, so we'd still
support current and previous stable version.

Reply to this email directly or view it on GitHubhttps://github.com//pull/334#issuecomment-36270614
.

@lo1tuma
Copy link
Member

lo1tuma commented Feb 27, 2014

It could probably work with at least node 0.8.19. But I don't see why we should support this.

@alexindigo
Copy link
Member

I just think imposing arbitrary limits is not the best way for a project to get a foothold in the node community. There are plenty of 0.8 in production at the moment and big chunk of 0.6s too.
I'm not advocating to keep that support at all costs, but if it's something we can do just by using different notation, why not? Node community is all about being inclusive, right? :)

@lo1tuma
Copy link
Member

lo1tuma commented Feb 27, 2014

We have never supported 0.8 because we and some of our dependencies make use of peerDependency which AFAIK was introduced in npm 1.2.10 (bundled with node 0.8.19).
This PR actually fixes the false promise we’ve made.

@spikebrehm
Copy link
Member

I actually do agree with @alexindigo on this one -- considering a recent version of 0.8 (0.8.19) works with peerDependencies, we may as well maintain support for it. The ^ notation is not enough to justify dropping 0.8. We could just change the engines field to:

 "engines": {
    "node": ">=0.8.19"
  },

@spikebrehm
Copy link
Member

Additionally, if we do decide to do this, probably it should come with 0.6.0, which very well might come out when Node 0.12.x is out

@lo1tuma
Copy link
Member

lo1tuma commented Feb 27, 2014

I’ve just tried the 05_requirejs example with node 0.8.19 and npm 1.2.10 and it fails on npm install

npm ERR! peerinvalid The package rendr does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer rendr-handlebars@0.2.0 wants rendr@>=0.5.0
npm ERR! peerinvalid Peer grunt-rendr-requirejs@0.1.1 wants rendr@~0.5

and

npm ERR! command "/usr/local/bin/node" "/usr/local/bin/npm" "install" "../../"

It seems the postInstall script introduced with #261 doen’t work as well with node 0.8 / npm 1.2.10.

@alexindigo
Copy link
Member

@lo1tuma Certain examples might have different requirements, since their setup is optional. And I'll take a look to see what it takes to support 0.8 in 05_requirejs. Thanx for pointing out the issue.

@lo1tuma
Copy link
Member

lo1tuma commented Feb 28, 2014

@alexindigo every app which uses rendr requires the peerDependency feature.

@c089
Copy link
Member Author

c089 commented Feb 28, 2014

Agreed, so we'll set the minimum to 0.8.19 and use ~ instead of ^ for now, right?

I think we can actually remove the engines property from the examples once we add it to rendr itself...

@alexindigo
Copy link
Member

If example app for some reason requires other minimal version of node, I think it's ok to state it clearly in package.json and README and people can choose what they want that particular feature or compatibility.

@c089
Copy link
Member Author

c089 commented Feb 28, 2014

Sure, I just think right now none of them does, right?

@alexindigo
Copy link
Member

Yep, just pointing out the use case, before we banned engine from examples' package.json for good. :)

@c089
Copy link
Member Author

c089 commented Feb 28, 2014

Agreed then. Will provide a new PR.

@c089 c089 closed this Feb 28, 2014
@spikebrehm
Copy link
Member

Thanks Chris!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants