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

refactor: superspawn usage #763

Closed
wants to merge 22 commits into from
Closed

refactor: superspawn usage #763

wants to merge 22 commits into from

Conversation

erisu
Copy link
Member

@erisu erisu commented Jan 8, 2020

Motivation and Context

Cleanup & use only superspawn everywhere.

Description

  • Replace child_process with superspawn
  • Remove direct requirement of Q dependency from cordova-ios
  • Keep using Q.progress which is available with superspawn.

Testing

  • npm t

Checklist

  • I've run the tests to see all new and existing tests pass

@erisu erisu added this to the 6.0.0 milestone Jan 8, 2020
@erisu erisu requested a review from raphinesse January 8, 2020 12:16
Copy link
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside my code comments, there's two remarks I would like to make:

Command printing was dropped without replacement and without a note in the PR description. I think we need either one.

Do we need to transform execa Errors to a format compatible to the old one (child_process or superspawn)? Did you check if there is any error handling that relies on special error properties or anything like that?

tests/spec/unit/lib/list-devices.spec.js Outdated Show resolved Hide resolved
tests/spec/unit/lib/list-devices.spec.js Show resolved Hide resolved
bin/templates/scripts/cordova/lib/versions.js Outdated Show resolved Hide resolved
bin/templates/scripts/cordova/lib/versions.js Outdated Show resolved Hide resolved
bin/templates/scripts/cordova/lib/versions.js Outdated Show resolved Hide resolved
bin/templates/scripts/cordova/lib/list-started-emulators Outdated Show resolved Hide resolved
bin/templates/scripts/cordova/lib/list-started-emulators Outdated Show resolved Hide resolved
bin/templates/scripts/cordova/lib/run.js Outdated Show resolved Hide resolved
bin/templates/scripts/cordova/lib/list-devices Outdated Show resolved Hide resolved
bin/templates/scripts/cordova/lib/list-devices Outdated Show resolved Hide resolved
bin/update Outdated Show resolved Hide resolved
bin/create Outdated Show resolved Hide resolved
@erisu erisu force-pushed the v6-execa branch 2 times, most recently from 97738da to 63ca5ae Compare January 9, 2020 00:19
@codecov-io
Copy link

codecov-io commented Jan 9, 2020

Codecov Report

Merging #763 into master will decrease coverage by 0.22%.
The diff coverage is 57.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #763      +/-   ##
==========================================
- Coverage   74.20%   73.98%   -0.23%     
==========================================
  Files          13       13              
  Lines        1849     1791      -58     
==========================================
- Hits         1372     1325      -47     
+ Misses        477      466      -11     
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/Podfile.js 73.80% <14.28%> (+0.22%) ⬆️
bin/templates/scripts/cordova/lib/run.js 21.10% <14.28%> (-0.33%) ⬇️
bin/templates/scripts/cordova/lib/check_reqs.js 51.94% <37.50%> (ø)
bin/templates/scripts/cordova/lib/build.js 50.51% <66.66%> (-0.26%) ⬇️
bin/templates/scripts/cordova/lib/prepare.js 82.05% <66.66%> (ø)
bin/templates/scripts/cordova/Api.js 71.32% <100.00%> (-0.11%) ⬇️
bin/templates/scripts/cordova/lib/listDevices.js 100.00% <100.00%> (ø)
...emplates/scripts/cordova/lib/listEmulatorImages.js 100.00% <100.00%> (ø)
bin/templates/scripts/cordova/lib/versions.js 100.00% <100.00%> (+9.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e65d685...17499fb. Read the comment docs.

@raphinesse
Copy link
Contributor

With the commits I just pushed we have resolved everything except for these two remarks I left before:

Command printing was dropped without replacement and without a note in the PR description. I think we need either one.

Do we need to transform execa Errors to a format compatible to the old one (child_process or superspawn)? Did you check if there is any error handling that relies on special error properties or anything like that?

@erisu erisu changed the title refactor: replace superspawn & child_process with execa WIP: refactor: replace superspawn & child_process with execa Jan 9, 2020
@erisu
Copy link
Member Author

erisu commented Jan 9, 2020

I will review everything again before answering.

It seems other changes will be needed anyways and more tests. I noticed that with the lack of tests it didn't catch something that would have become an issues after dependency changes.

Depending on timing, I am expecting maybe by late-Jan or early-Feb before I can answer and have all the fixes and new tests completed.

IF others need to push on with the major release, I recommend ignoring this PR for now and will target next next major.

@erisu erisu force-pushed the v6-execa branch 2 times, most recently from b7c40dc to 124e187 Compare January 10, 2020 01:28
@raphinesse
Copy link
Contributor

This PR should probably renamed to "drop Q" or something like it

@erisu erisu changed the title WIP: refactor: replace superspawn & child_process with execa WIP: refactor: drop q & replace superspawn/child_process with execa Jan 12, 2020
@erisu erisu changed the title WIP: refactor: drop q & replace superspawn/child_process with execa refactor: superspawn usage Feb 14, 2020
@erisu erisu requested a review from raphinesse February 14, 2020 10:04
@erisu
Copy link
Member Author

erisu commented Feb 14, 2020

@raphinesse PR has been finalized and ready to review.

  • Everything will use superspawn.
  • cordova-ios direct requirement for Q dependency has been removed.
  • Q.progress will remain to be used and accessible by superspawn.

@erisu erisu dismissed raphinesse’s stale review May 18, 2020 03:06

needs a re-review

@erisu erisu requested review from dpogue and purplecabbage May 18, 2020 03:06
erisu and others added 21 commits May 20, 2020 11:57
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
@erisu
Copy link
Member Author

erisu commented May 20, 2020

PR no longer valid. Replaced with #859 & #860

@erisu erisu closed this May 20, 2020
@erisu erisu deleted the v6-execa branch May 27, 2020 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants