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

Updated readable-stream to v2.2.6 #1677

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Contributor

As titled.

@AdriVanHoudt
Copy link

It might be useful to enable something like greenkeeper?

@dougwilson
Copy link
Member

I haven't upgraded it because it's a breaking change... You can see the CI is broken.

@mcollina
Copy link
Contributor Author

I think it is a problem with npm during the install step. readable-stream works fine on node 0.8 with this config https://github.com/nodejs/readable-stream/blob/master/.travis.yml, I'll see if that works here as well, or if there is an easy fix on readable-stream side.
I wonder how readable-stream 1.x works on Node v0.6.

Have you got any plans to remove support for 0.6 and 0.8?

@dougwilson
Copy link
Member

Hi @mcollina it's still a breaking change if it's going to require users to upgrade their npm version just like it would be if we required then to upgrade their Node.js version.

Have you got any plans to remove support for 0.6 and 0.8?

I didn't even want to support Node.js 0.6 but added it because of the barrage of Node.js 0.6 issues that coming in because there were OS distros that only had 0.6 in their package manager, and of course this being a low level driver modules, apparently it is very popular among those types of users.

I plan to drop them when 3.0 comes out, but that just needs myself or others to work on the issues tagged with 3.0 milestone for some of the "big bugs" that are lying around.

@mcollina
Copy link
Contributor Author

@dougwilson would you mind restarting the build, now readable-stream  just depends using ~, so the problem we had was fixed.

@dougwilson
Copy link
Member

Hi @mcollina thanks for the update! I restarted the Travis CI build but it still fails.

@mcollina
Copy link
Contributor Author

@dougwilson travis is passing with readable-stream  v2.2.9.
AppVeyor is failing, but it seems unrelated: https://ci.appveyor.com/project/dougwilson/node-mysql/build/614/job/tg48p3a3xe7639ag.

@dougwilson
Copy link
Member

Hm, I wonder what changed to increase the flakiness of that test. I fixed it on master, so am merging your PR :) ! I assume that even though this is a major version bump of readable-stream, it won't actually cause any backwards compatibility issues within how we are exposing the streams to users, right?

@mcollina
Copy link
Contributor Author

It is fully backward-compatible. The difference is that readable-stream v1 is streams 2, while readable-stream v2 is streams 3.

@dougwilson
Copy link
Member

Awesome! I'm just trying to determine what I need to add to the change log and what to change in the README regarding this.

@dougwilson
Copy link
Member

@mcollina I'm reverting this because readable-stream just introduced another dependency with a ^ 4 days ago. I don't think that module is willing to provide the version support we are looking for, sorry.

@mcollina
Copy link
Contributor Author

mcollina commented Jun 6, 2017

@dougwilson I'll get you a new version. I'm not sure how I can test that still works on .travis.yml

@dougwilson
Copy link
Member

Yea, if you're using a test framework that doesn't care about old version support, you can't test on old versions. Your testing framework is the largest limiter for the version you can test against. Being from Perl, it's just mind-boggling that tests frameworks in Node.js don't strive to support all possible versions, instead typically only supporting very recent versions, driving modules that use them to have no choice in the versions they can test on.

@mcollina
Copy link
Contributor Author

mcollina commented Jun 6, 2017

version 1 of readable-stream is unmaintained and ideally unsopported. We cannot test it on CITGM in core as well.

The problem is not really the test framework. The problem is npm itself.

@dougwilson
Copy link
Member

I thought you were referring to testing the current readable-stream against things like Node.js 0.8 on Travis CI.

@dougwilson
Copy link
Member

Doing some tests, looks like even through I pinned the specific version of readable-steam in this PR, the reason why mysql is no longer installable is because it depends on a range for string_decoder and that module also introduced a ^ dependency here: https://github.com/rvagg/string_decoder/blob/master/package.json#L7

@mcollina
Copy link
Contributor Author

mcollina commented Jun 6, 2017

a new version of both is coming.

@mcollina
Copy link
Contributor Author

mcollina commented Jun 6, 2017

@dougwilson it should be fixed, a new version of string_decoder has just been released.

@dougwilson
Copy link
Member

Sweet, the string_decoder update fixed the current master version right away :)

@mcollina
Copy link
Contributor Author

mcollina commented Jun 6, 2017

readable-stream with 2.2.11 depends on safe-buffer using ~ as well.

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

Successfully merging this pull request may close these issues.

3 participants