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

WORKAROUND SOLUTION for local grunt issue in cordova-js #174

Conversation

brodycj
Copy link

@brodycj brodycj commented Jun 22, 2018

Platforms affected

All

What does this PR do?

Resolves the following issue:

If I would try coho prepare-platform-release-branch or coho copy-js it stops with an error like this:

cordova-js/ ==================== Executing: grunt compile:android --platformVersion=7.1.1-dev
grunt-cli: The grunt command line interface (v1.2.0)

Fatal error: Unable to find local grunt.

If I would try npm install in cordova-js it does not resolve the issue. coho seems to clean node_modules before attempting grunt compile:android.

Ugly workaround was to commit node_modules in my local (personal) master branch of cordova-js (NOT in the apache repo) before running coho prepare-platform-release-branch or coho copy-js.

The update proposed here is a workaround solution that automatically does npm install in cordova-js to solve the grunt issue described here.

Moving forward:

I would like to see at least one other member try this change before integrating into master. I have not tried it with more than one platform at a time (not sure if I would favor updating more than one platform at a time anyway).

I think a much nicer solution would be for us to move on to a more modern solution such as WebPack, microbundle, or maybe even Gulp instead of Grunt.

What testing has been done on this change?

Both ./cordova-coho/coho copy-js -r android and ./cordova-coho/coho prepare-platform-release-branch --version 8.0.0-dev -r android work as expected for me.

(I generally use a local clone of the cordova-coho repo for coho tasks.)

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@brodycj brodycj added the bug label Jun 22, 2018
@brodycj brodycj mentioned this pull request Jul 4, 2018
2 tasks
@brodycj
Copy link
Author

brodycj commented Jul 4, 2018

This workaround solution is now included by the changes in PR #176. I think it would be much better to have a more "real" solution. As promised in #176 here is my explanation of the issues encountered here:

I think there are 3 issues to deal with here:

  1. global grunt installation is needed, which I think is not desired
  2. grunt needs local grunt in cordova-js for coho prepare-platform-release-branch and coho copy-js to work
  3. package-lock.json artifact in cordova-js, generated by newer versions of npm is not either committed or managed by .gitignore in cordova-js

This workaround is needed to deal with issue 2. Not sure if this is the best solution for the future or not.

It should be possible for this package to include grunt in devDependencies and use it from local scripts to resolve issue 1. I hope I can investigate this one (someday).

I think the right solution to issue 3 would be to commit package-lock.json in cordova-js. Maybe coho should be updated to delete other generated artifacts (node_modules and pkg) in cordova-js as needed. I think package-lock.json should be committed in a major version bump in cordova-js to be on the safe side. (Workaround to issue 3 is to manually delete package-lock.json and other generated artifacts whenever needed.)

@brodycj
Copy link
Author

brodycj commented Jul 27, 2018

Now part of #176

@brodycj brodycj closed this Jul 27, 2018
@brodycj brodycj deleted the cordova-js-local-grunt-workaround-solution branch November 30, 2018 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant