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

CB-14064: Dependency updates & drop node4 CI #15

Merged
merged 3 commits into from May 18, 2018

Conversation

Projects
None yet
4 participants
@dpogue
Copy link
Member

dpogue commented May 4, 2018

Also added the pull request issue template (copied from cordova-lib) since I noticed that was missing

@dpogue dpogue requested a review from stevengill May 4, 2018

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented May 4, 2018

@dpogue The update of shelljs breaks the AppVeyor build on Windows. shell.which('ls') seems to return something unexpected here.

@dpogue

This comment has been minimized.

Copy link
Member

dpogue commented May 4, 2018

Yeah, I'm going to get a Windows VM set up to run the tests locally on Windows and see what's going on. I'd really like to not to be stuck on an old version of shelljs :\

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented May 4, 2018

I got a dual boot setup here, let me take a quick look at it.

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented May 4, 2018

Well basically, shelljs returns ShellString instances, while pathname.extname expects a plain string. This behavior is not Windows specific either. It's just that the problematic code in superspawn.js only is active on Windows.

So we only need to unwrap the encapsulated string. If I replace the line I linked above with the following, all tests pass on my Windows installation using node 8:

cmd = String(shell.which(cmd) || cmd);

@dpogue dpogue force-pushed the dpogue:dependency-updates branch 2 times, most recently from ce87065 to 762c7fe May 5, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 5, 2018

Codecov Report

Merging #15 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #15   +/-   ##
=======================================
  Coverage   85.23%   85.23%           
=======================================
  Files          19       19           
  Lines        1768     1768           
  Branches      376      376           
=======================================
  Hits         1507     1507           
  Misses        261      261
Impacted Files Coverage Δ
src/superspawn.js 69.41% <0%> (ø) ⬆️

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 8d6511e...74d1341. Read the comment docs.

dpogue added some commits May 4, 2018

@dpogue dpogue force-pushed the dpogue:dependency-updates branch from 762c7fe to 74d1341 May 15, 2018

@dpogue dpogue changed the title Dependency updates & drop node4 CI CB-14064: Dependency updates & drop node4 CI May 15, 2018

@stevengill
Copy link
Contributor

stevengill left a comment

LGTM. Merge it

@dpogue dpogue merged commit a452993 into apache:master May 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dpogue dpogue deleted the dpogue:dependency-updates branch May 18, 2018

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