CB-14242 remove bundledDependencies and committed node_modules #388
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #388 +/- ##
=======================================
Coverage 74.29% 74.29%
=======================================
Files 12 12
Lines 1564 1564
=======================================
Hits 1162 1162
Misses 402 402Continue to review full report at Codecov.
|
erisu
approved these changes
Aug 2, 2018
There was a problem hiding this comment.
Everything appears OK to me. The only thing that I noticed when looking at the diff between npm i with and without pre-checked in packages were a few sub deps upgraded.
The plist package was the only one that had a major update (1.2.0 -> 2.1.0), while the rest were minors or patches.
Seeing that the tests are passing, It shouldn't be an issue.
janpio
moved this from Untriaged
to Waiting for Review
in Apache Cordova: Platform Pull Requests
janpio
moved this from ⏳ Waiting for Review
to ✅ Approved
in Apache Cordova: Platform Pull Requests
janpio
moved this from ✅ Approved, waiting for Merge
to 💥 Blocked: Conflict
in Apache Cordova: Platform Pull Requests
6 tasks
raphinesse
approved these changes
Sep 11, 2018
Apache Cordova: Platform Pull Requests
automation
moved this from 💥 Blocked: Conflict
to ✅ Approved, waiting for Merge
Sep 11, 2018
Apache Cordova: Platform Pull Requests
automation
moved this from ✅ Approved, waiting for Merge
to 🏆 Merged, waiting for Release
Sep 11, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Platforms affected
iOS
What does this PR do?
See https://issues.apache.org/jira/browse/CB-14242:
bundledDependenciesentry frompackage.jsonnode_modules.gitignoreto completely ignorenode_modulesSuggested merge type
I would personally favor rebase merge: would be much cleaner for limited number of commits.
Squash merge is not desired since one of the commits is extremely large.
What testing has been done on this change?
cordova platform add brodybits/cordova-ios#cb-14242-remove-bundled-dependencies-and-node-moduleson new Cordova project and run on iOS using Cordova CLInpm testitems pass on Travis CIChecklist
Added automated test coverage as appropriate for this change.