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

Swap npm-run-all for concurrently #488

Merged
merged 2 commits into from
Nov 2, 2017

Conversation

samit4me
Copy link
Contributor

This should fix #120.

Have tested this on

  • macOS 10.12.6 && node 6.10.3 && npm 5.4.2 && yarn 1.2.1.
  • windows 10 v1703 && node 6.9.1 && npm 3.10.8 && yarn 1.2.1.

Have also test that testing and linting work as expected (with and without errors) on both of the above.

Would be fantastic if others that have had issues in the past could pull this locally and test.

@coveralls
Copy link

coveralls commented Oct 19, 2017

Coverage Status

Coverage remained the same at 90.476% when pulling 014ce43 on samit4me:fix-start-script-issues into 4fad0e5 on coryhouse:master.

@kwelch
Copy link
Collaborator

kwelch commented Oct 20, 2017

This builds, tests, and runs properly. It also resolves #120 as expected.

I am good with it.

@coryhouse any reason to not use concurrently, instead of npm-run-all

@nickytonline
Copy link
Collaborator

@kwelch, CI is breaking though. I haven't had a chance to see why though.

@kwelch
Copy link
Collaborator

kwelch commented Oct 20, 2017

It is actually a yarn issue. If a package has the engine field set yarn will not install on that version. It started back with the react@16 merge.

I would hold off on the merge until we can get some results from another Windows machine though (maybe?).

@samit4me
Copy link
Contributor Author

samit4me commented Oct 20, 2017

Looking at the changes in package-lock.json and yarn.lock it looks like they haven't been updated in sometime. In master autoprefixer is 7.1.4 listed in package.json, but it is 7.1.2 in package-lock.json.

https://github.com/coryhouse/react-slingshot/blob/4fad0e5470f51fc493b05aa8a8e103e0622db24d/package.json
https://github.com/coryhouse/react-slingshot/blob/4fad0e5470f51fc493b05aa8a8e103e0622db24d/package-lock.json

Did you want me to roll back all the unrelated changes in package-lock.json and yarn.lock and try CI again?

@samit4me
Copy link
Contributor Author

The issue looks related to rst-selector-parser which was added into yarn.lock as a dependancy of enzyme 3

https://github.com/coryhouse/react-slingshot/pull/488/files#diff-8ee2343978836a779dc9f8d6b794c3b2R2308

@samit4me
Copy link
Contributor Author

Okay so I've looked into this a little bit more and it looks like Enzyme swapped out scapel for rst-selector-parser in PR 1086 and rst-selector-parser which requires node 5 or later see here.

This change looks like it was released with v3 of enzyme, so I looked into when this repo was updated to v3 and it seems this was done in #480. This is further confirmed with the CI failures in that PR, see details.

Given that enzyme doesn't support node 4 and it is no longer active according to the node release schedule should/can this repo drop support for node 4? If so, let me know I'd be more than happy to help where possible.

@samit4me samit4me mentioned this pull request Oct 20, 2017
@coryhouse
Copy link
Owner

I'm open to dropping support for Node 4.

Appreciate the PR! However, I'm hesitant to move to concurrently since its API is uglier.

@samit4me
Copy link
Contributor Author

Yeah agree, concurrently is uglier :)

Looks like there might be a fix for node 4 soon, see enzymejs/enzyme#1285

@samit4me
Copy link
Contributor Author

The CI should now pass as the upstream issue has been resolved. Is anyone able to re-run the failed ones?

@kwelch
Copy link
Collaborator

kwelch commented Oct 22, 2017

I was able to restart Travis, but not AppVeyour

@coveralls
Copy link

coveralls commented Oct 22, 2017

Coverage Status

Coverage remained the same at 90.476% when pulling a0bbd7b on samit4me:fix-start-script-issues into 4fad0e5 on coryhouse:master.

@samit4me
Copy link
Contributor Author

Thank you @kwelch, it looks like the lock files needed updating anyways to take advantage of the upstream fix. Looks like Travis CI is passing now 🎉 we can only hope appveyor does too when if finishes.

@samit4me
Copy link
Contributor Author

Everything seemed to pass, but babel-node tools/testCi.js appeared to timeout on node 7, very strange.

@samit4me
Copy link
Contributor Author

Is concurrently a no go? I totally agree it's much uglier than run-npm-all but I find this is easier to live with than needing to close the console when ctrl+c doesn't work. If this is a no go, might have to look into a different solution.

@coryhouse
Copy link
Owner

I'm on Mac so I can't easily test at the moment, but if @nickytonline and @kwelch are good w/ the switch, I am so Windows people have a better experience

@nickytonline
Copy link
Collaborator

I'm on a Mac only as well at the moment. Haven't had a chance to set up my IE and Edge VMs yet on the new machine.

@corlaez
Copy link

corlaez commented Nov 2, 2017

I can confirm the issue in windows 10. currently doing:

netstat -ano | findstr :PORT
taskkill /PID PID_NUMBER /F

@coryhouse coryhouse merged commit bfe0c4d into coryhouse:master Nov 2, 2017
@coryhouse
Copy link
Owner

Merged. Thanks for the PR!

@coryhouse
Copy link
Owner

I just reverted this commit to resolve #500

@coryhouse
Copy link
Owner

Sorry, my mistake, the actual issue was caused by #497. I just re-implemented this PR. Thanks again!

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.

Killing start script requires hitting Ctrl+C twice
6 participants