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

Fix npm link usage on Node.js 6 #815

Merged
merged 3 commits into from
May 8, 2016

Conversation

jamestalmage
Copy link
Contributor

Fixes #814, but causes all sorts of test breakage.

I can't really figure out why it breaks the tests, it seems to work fine IRL. My only guess is that being wrapped by tap is somehow causing the problem.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @ariporad, @ingro and @naptowncode to be potential reviewers

@jamestalmage
Copy link
Contributor Author

Tests fixed

@jamestalmage jamestalmage changed the title temp fix for npm link Fix npm link usage on Node.js 6 May 7, 2016
@novemberborn
Copy link
Member

What if we rewrite the require('ava') call in the workers to be a real path?

I'm not sure about how often Node.js releases occur, do we really need to land this? I doubt we'd want to maintain this for the early Node 6 releases. On the other hand it'll help us during development, so maybe that's OK.

@jamestalmage
Copy link
Contributor Author

My thought is to merge and track Node 6 progress. Once the old behavior is restored and stable, revert this out.

The code I've copied here out of child_process has remained pretty stable, so I don't think we're in danger of missing out on any updates in the core module.

Still, I agree, this isn't something I want to maintain long term, hence all the extra comments letting us know what and when to revert.

I don't want to wait for Node to sort this out before I can use npm link again.

@sindresorhus sindresorhus merged commit 1368d0e into avajs:master May 8, 2016
@sindresorhus
Copy link
Member

👍

@dcousineau
Copy link
Contributor

dcousineau commented May 12, 2016

When running this fix on our test suite, nyc drops reported coverage from 35.95% (on v0.14.0) to 1.82% (running HEAD from master).

Not sure if you were aware of this but you may want to delay publishing to NPM.

@jamestalmage
Copy link
Contributor Author

@dcousineau - Good Catch!

#827 should fix your issue. NYC still will not work if you are using npm link ava on Node 6. But it will always work on all earlier versions of Node, and will also work on Node 6 as long as you haven't npm linked AVA.

@jamestalmage
Copy link
Contributor Author

FYI: The NYC issue is because spawn-wrap does not support the -e flag.

I have opened istanbuljs/spawn-wrap#22, requesting that support for -e be added, but #827 works to solve this issue.

@dcousineau
Copy link
Contributor

Makes sense. I'm not using npm link anywhere becuase I keep forgetting its a thing :P

jamestalmage added a commit to jamestalmage/ava that referenced this pull request May 23, 2016
This mostly reverts avajs#815, and avajs#827. The Node regressions they fixed were resolved in Node 6.2.0. The new behavior is just to print a warning message so people know how to fix it.
jamestalmage added a commit to jamestalmage/ava that referenced this pull request May 23, 2016
This mostly reverts avajs#815, and avajs#827. The Node regressions they fixed were resolved in Node 6.2.0. The new behavior is just to print a warning message so people know how to fix it.
sindresorhus pushed a commit that referenced this pull request May 24, 2016
This mostly reverts #815, and #827. The Node regressions they fixed were resolved in Node 6.2.0. The new behavior is just to print a warning message so people know how to fix it.
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.

None yet

5 participants