coopernurse / node-pool

Generic resource pooling for node.js
2.38k stars 259 forks source link

v2.4.0 #115

Closed sandfox closed 8 years ago

sandfox commented 8 years ago

Just putting this to add some organisation and avoid my poor memory making a mess of things

@felixfbecker - does this rough order of merging make sense?

  1. [ ] do/merge #75
  2. [x] do/merge #111 / #114
  3. [x] do/merge #110
  4. [x] do/merge #113 (there will probably be a bunch of readme changes by this point)
felixfbecker commented 8 years ago

I would say the order is pretty irrelevant. I would count on you to rewrite the tests as it is your code in the end and I dont understand it completely. This package is pretty important (Sequelize ORM for example depends on it) and it should have good coverage.

On afterthought, maybe it would make sense to define a basic eslint style before rewriting the tests. Btw, mocha requires at least 0.8, or if you wanna install an old version 0.6. So we have to raise the Node requirement at least a little bit, which means this would be 3.0. People who are still on 0.2 (I mean, come on!) could continue to use 2.x. But because of potentially slow adoption of a major version I would say first merge #114, leave the tests as is and release 2.4. Then rewrite tests for 3.0 and raise requirement to 0.6 (this is also the version Sequelize needs).

sandfox commented 8 years ago

Sorry, I totally mis-read/understood your comment on #114 and thought we'd have to wait till the rewritten tests land before merging it, forgot about travis (or the fact I could have run them) :grimacing:

That bit of daftness aside...

I'll merge that in along with something for #113 and then #110 is probably fine to do.

If you think it's safer I'll bump the test rewrite to back v3. I don't know if I consider tests as in scope for the semver versioning, as changing the test library only affects people wanting to develop rather than use the library.

As you say major version upgrades take time to propagate (although I've noticed that sequalize uses greenkeeper, and it's easy enough to open a PR against pg who is another large user of the lib) so I'm very keen to try and put as much as possible into the same major version.

felixfbecker commented 8 years ago

The problem is that when your test framework requires 0.6, you cannot test (on Travis) anymore for node versions below that. You can only test for >0.6, which means you effectively cannot support 0.2 anymore, because any addition could break it without you noticing it. Except of course if you keep the tests for 0.2 in a seperate folder and somehow tell Travis to use that for 0.2.

sandfox commented 8 years ago

ah right gotcha, for some reason I imagined travis went back all the way to 0.2. For the sake of being nice I guess it that should be major version upgrade for the test changes then, we'll just need to stick a big "Everything will fine" notice on it in the hope everyone will upgrade :-)

felixfbecker commented 8 years ago

@sandfox I think I have the perfect solution. How about this as npm test?

"test": "sh -c \"semver $(node -v) -r '<0.2' && expresso -I lib test/*.js || mocha\""
sandfox commented 8 years ago

Oh god I'm being slow at the moment (or at least reading things properly/slowly), finally grasped the node / test-framework versions thing.

While I like the test lib switch in theory, I feel like we're heading into the valley of complexity.

Having dug through the project history and previous travis support I've realised the node 0.6 and 0.8 support was totally arbitrary, and node version support has been mostly unspecified.

Whilst we could go with bumping major version and I feel like it can't be hard to find every library that depends on this one in npm and send a pull request in some semi-automated fashion, thats a yak shave I'd rather save for later.

I'm going to make an executive decision (which I may come to regret/revert) to bin explicit pre-0.8 support in the current major version (dropping 0.8 seems a little too aggressive) so we can ship this useful stuff.

sandfox commented 8 years ago

(and I think the "node 0.2" as seen in the package.json was almost kind of arbitrary and very untested)

felixfbecker commented 8 years ago

Alright :)

sandfox commented 8 years ago

going to shift test changes and some other stuff to 2.4.1