hapijs / glue

Server composer for hapi.js
Other
245 stars 62 forks source link

Breaking changes on 2.3.0 and 2.4.0 #40

Closed rodfernandez closed 8 years ago

rodfernandez commented 9 years ago

Hi,

There are a couple of commits (https://github.com/hapijs/glue/commit/e3b7da5bfc2ab4f571d78f4254a89eccac3c7797 and https://github.com/hapijs/glue/commit/3c8c1e09657987a0be5a0b98ae545ada38a0e94f) that introduced breaking changes when allowing hapi>=10, since those versions require node>=4.

Applications that depend on glue@2 but still run on node@0.x (I know, enterprise) will break if re-installed, since the latest versions of hapi are using certain ES2015 features.

I would like to suggest bumping the major version and unpublishing those versions from npm.

Cheers!

lloydbenson commented 9 years ago

Doesn't the || clause say that you CAN use the new one but the old one is still valid?

rodfernandez commented 9 years ago

Hi LLoyd! After a fresh npm install, I see hapi 11 there. Of course, no damage if the application has a shrikwrap.json targeting hapi < 10 , but you know... :wink:

BTW, this will impact rejoice's versioning.

lloydbenson commented 9 years ago

of course but if you have hapi < 10 in your app, and have this glue version it should still install fine?

lkptrzk commented 9 years ago

i got bit by this as well. might not be the intended usage, but i wasn't including hapi in the package.json, only glue.

lloydbenson commented 9 years ago

I personally think we should have a major breaking change for hapi 10.x anyway so we can start using es6. But I am just trying to explain why the maintainer may not consider this a breaking change.

lloydbenson commented 9 years ago

You could fix that by including hapi in your package.json file right?

rodfernandez commented 9 years ago

Correct, but it feels like a workaround. I will never understand semver...

lkptrzk commented 9 years ago

perhaps, i just switched the dep to be explicitly 2.2.0 (instead of ^2.2.0 or whatever i had it), haven't had time to circle back on how that behaves (instead i'm just working on upgrading node + npm etc).

if i'm supposed to be explicitly including hapi in the package.json, then the problem is my incorrect understanding of how to use glue, and it'd be awesome to get some context on why that is. if you're supposed to be able to use glue without specifying hapi in the package.json, then i'd consider this a breaking change.

lloydbenson commented 9 years ago

fair point.

lloydbenson commented 9 years ago

hopefully the maintainer will address these questions.

gergoerdosi commented 9 years ago

@rodfernandez, @lkptrzk: I recommend using http://semver.npmjs.com/:

screen shot 2015-11-06 at 23 59 34 screen shot 2015-11-07 at 00 00 26
lkptrzk commented 9 years ago

i think a concrete example will help -

using this server.js file:

var Glue = require('glue');

var manifest = {
  server: {},
  connections: [ { port: 3000 } ]
};

Glue.compose(manifest, {}, function (err, server) {

  if (err) {
    throw err;
  }

  console.log(server.version);
});

and this package.json:

{
  "name": "glue-bug",
  "version": "1.0.0",
  "dependencies": {
    "glue": "2.2.0"
  }
}

if you npm install && node server.js, the output is 9.3.1.

with this package.json:

{
  "name": "glue-bug",
  "version": "1.0.0",
  "dependencies": {
    "glue": "2.3.0"
  }
}

the output from the same script is 10.5.0.

a minor bump in glue caused a major bump in hapi

lkptrzk commented 9 years ago

that specific example used npm 3.3.8 and node v0.10.40

csrl commented 9 years ago

Yes, if you haven't explicitly called out the version of Hapi you want to use, then glue will pull in the latest. To that effect, glue 2.2.0 which allowed Hapi 9 would also be a breaking change. Basically any time Hapi major version is bumped, would require a major version bump of Glue if we took that approach. I've always considered that one should explicitly require a Hapi version in their project.

The README should get updated with this expectation.

I'm open to alternate approaches and suggestions. But I'm not inclined to bump Glue's major version just to add support for the next major version of Hapi. I do expect to move to requiring Node 4+ and Hapi 11 (or whatever the version of the week is) in Glue 3.0 as part of issue 31.

rodfernandez commented 9 years ago

A warn in the documentation is a great solution!

csrl commented 8 years ago

Commit db5cf3a resolves this.

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.