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

GH-540: Fix copy-www-build-step without shelljs #600

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Apr 16, 2019

Platforms affected

iOS

Motivation and Context

See #540

Description

There didn't seem to be a compelling reason to keep this as JavaScript since it was primarily just shelling out to run various commands. By keeping it in bash, we can significantly simplify the invocation in the xcodeproj file and we can run it without a dependency on shelljs.

I also moved it into a Scripts folder inside the project so that our xcodeproj file isn't reliant on the cordova/lib folder existing, and this particular script isn't actually a "lib" in the standard sense.

Testing

Created a local project with this branch and built the iOS platform both with and without node_modules present.
All automated tests passed locally.

Checklist

  • I've run the tests to see all new and existing tests pass
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)

There didn't seem to be a compelling reason to keep this as JavaScript
since it was primarily just shelling out to run various commands. By
keeping it in bash, we can significantly simplify the invocation in the
xcodeproj file and we can run it without a dependency on shelljs.

I also moved it into a Scripts folder inside the project so that our
xcodeproj file isn't reliant on the cordova/lib folder existing, and
this particular script isn't actually a "lib" in the standard sense.
@dpogue dpogue added this to the 5.0.1 milestone Apr 16, 2019
@codecov-io
Copy link

codecov-io commented Apr 16, 2019

Codecov Report

Merging #600 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #600   +/-   ##
=======================================
  Coverage   74.58%   74.58%           
=======================================
  Files          11       11           
  Lines        1826     1826           
=======================================
  Hits         1362     1362           
  Misses        464      464

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 44899e2...83c2408. Read the comment docs.

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

👍

@dpogue dpogue merged commit a79f64e into apache:master Apr 17, 2019
@dpogue dpogue deleted the copy-www-build-step branch April 17, 2019 00:00
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.

4 participants