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 glob for node 6 compatibility #2234

Closed
wants to merge 1 commit into from

Conversation

kornelski
Copy link

@kornelski kornelski commented Apr 30, 2016

Older version of glob uses a version of graceful-fs, which is deprecated and incompatible with node v6.

@calebmer
Copy link

calebmer commented May 1, 2016

This also removes a graceful-fs deprecation warning for those with OCD that are don't like that type of thing.

@leipert leipert mentioned this pull request May 2, 2016
@dasilvacontin dasilvacontin added this to the v3.0.0 milestone May 3, 2016
@dasilvacontin
Copy link
Contributor

Thanks! It'll have to wait – cannot bump in mocha v2 for the same reason we cannot bump jade: #2200 (comment).

@kornelski
Copy link
Author

kornelski commented May 3, 2016

I admire that you strictly respect semver and still support node 0.8.x users, even though it's not even maintained by the node team.

However, mocha is also very important on the cutting edge part of the node ecosystem.

To help adoption of Node 6, and also reduce potential for breakage for those upgrading to Node 6, and reduce pressure on you releasing a new major release, could you release just that small change as mocha 3.0.0 (bumping version just to satisfy semver for node 0.8 users), and rename the currently planned v3.0.0 to v4.0.0?

@dasilvacontin
Copy link
Contributor

@pornel I was actually thinking of doing something like that, or maybe v3.0.0-alpha or the sorts – thanks for your voice! Making Mocha run on Node v6 is important imo, and you can tell other users think so as well.

What do you peps think, @mochajs/mocha?

@ScottFreeCode
Copy link
Contributor

Try npm install mochajs/mocha to use the latest master from GitHub and see if that still has the issue? It looks like #2201 should have eliminated the graceful-fs dependency by updating glob to 3.2.11, it just hasn't made it into a release yet. Of course, one would want to use an actual release of Mocha (at least in production); if I'm not mistaken the changes from #2201 will be able to go into a 2.4.6 or 2.5.0 release, without breaking compatibility, once #2079/#2231 is done.

That said, as a user I'm all for having an early copy of the next "major" version though (which should be able to resolve the Jade warning among other things; updating to the latest version of all dependencies if possible might be a good idea at that point), if it isn't too much trouble to maintain both that and the current 2.x.y release line until the Mocha team is ready to switch over fully.

@boneskull
Copy link
Contributor

@ScottFreeCode Seems to be correct that Node.js v6.x is not broken when installing from master, but is from the currently published v2.4.5 (though I have nothing to confirm this except the warning when installing).

Installing from master against Node.js v0.8 yields no errors or warnings.

This leads me to believe we could release v2.4.6 with an upgraded graceful-fs. I am extremely hesitant to do this until we get automated browser tests in place. I thought using the "CLI-only" chalk module would not affect browser users; I was incorrect, which is the reason for my hesitation. Nothing can be so easily assumed when bundling for the browser.

If I can confirm graceful-fs causes a Mocha installation to break--not just yield an install warning--I'll investigate doing a patch release later today; at minimum, we'll want to do some manual browser tests, and update our CI matrix.

cc @mochajs/mocha

@boneskull
Copy link
Contributor

Going to close this PR if that is the case; if that is the case, the problem has already been solved.

@boneskull boneskull removed the TO-MERGE label May 4, 2016
@boneskull boneskull removed this from the v3.0.0 milestone May 4, 2016
@boneskull
Copy link
Contributor

Can anyone confirm a case where Mocha breaks under Node.js v6? Our test suite passes with Node.js 6.

I'm having trouble understanding the trail of issues, discussion and code surrounding the deprecation of old graceful-fs versions. Does it actually break, or will it actually break in Node.js v7?

See isaacs/node-graceful-fs#58 prompting the deprecation warning, and the linked issue in nodejs/node#5213.

Hopefully one of @isaacs @ChALkeR @jasnell or @thealphanerd can help distill the conversation around those issues...? Thank you, if you can.

@ChALkeR
Copy link

ChALkeR commented May 5, 2016

@boneskull The deprecation warning upon installation in graceful-fs isn't very precise — it still works with Node.js 6 due to a temporary fix that was landed on Node.js side just for that, see nodejs/node#5102.

That temporary work-around is going to be reverted in Node.js 7.0 (refs: nodejs/node#5213, nodejs/node#6413).

Note that the fix also prints a warning in runtime, from Node.js side, and that warning is printed only in those situations that will throw in 7.0.

@ChALkeR
Copy link

ChALkeR commented May 5, 2016

I opened isaacs/node-graceful-fs#64 in regards to the deprecation message.
The initial graceful-fs deprecation message was fine, but it was mistakenly changed in isaacs/node-graceful-fs#61.

Update: the deprecation message in graceful-fs was fixed.

@boneskull
Copy link
Contributor

@ChALkeR Thanks for the explanation. I can confirm the warning is now fixed.

@boneskull
Copy link
Contributor

To summarize for interested parties:

  1. [email protected], which current [email protected] uses, does not actually break on Node.js v6, as you will see from the updated deprecation message of its dependency, graceful-fs.
  2. [email protected] should break under Node.js v7 in certain (?) circumstances.
  3. We updated to [email protected], which should not break under Node.js v7, but have not released this change.
  4. Since nothing is actually breaking right now (there is no Node.js v7 yet), I'm not in a hurry to release anything.
  5. So, I'm going to close this issue, and we'll release a new patch version with some recent fixes (including the upgraded [email protected]) once necessary changes for Make CI run browser tests #2231 land.

@boneskull boneskull closed this May 5, 2016
@boneskull
Copy link
Contributor

(warnings can be hidden during npm install by using --loglevel=error or configured permanently in .npmrc, if it's bothering you too much)

@darkmorpher
Copy link

darkmorpher commented May 21, 2016

Related #2262

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.

7 participants