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

Improve plugman/uninstall.js messages #630

Merged
merged 2 commits into from
Jul 31, 2018
Merged

Improve plugman/uninstall.js messages #630

merged 2 commits into from
Jul 31, 2018

Conversation

janpio
Copy link
Member

@janpio janpio commented Jul 30, 2018

Currently you get an error message like this if you try to uninstall a plugin while an unsupported platform (in this case a folder called .git) is present:

(node:7428) UnhandledPromiseRejectionWarning: CordovaError: .git not supported.

The error message doesn't make it obvious that this is about a "platform". This PR fixes this to be more clear.

While fixing this one case, I also encountered other messages in the same file that could be more precise and include platform/plugin in the output, so I changed those as well.


Complete output from where I encountered it:

λ ionic cordova plugin remove cordova-sqlite-storage
> cordova plugin remove cordova-sqlite-storage --save
(node:7428) UnhandledPromiseRejectionWarning: CordovaError: .git not supported.
    at Function.module.exports.uninstallPlatform (C:\nvm\v8.11.3\node_modules\cordova\node_modules\cordova-lib\src\plugman\uninstall.js:76:25)
    at C:\nvm\v8.11.3\node_modules\cordova\node_modules\cordova-lib\src\cordova\plugin\remove.js:76:50
    at _fulfilled (C:\nvm\v8.11.3\node_modules\cordova\node_modules\cordova-lib\node_modules\q\q.js:787:54)
    at self.promiseDispatch.done (C:\nvm\v8.11.3\node_modules\cordova\node_modules\cordova-lib\node_modules\q\q.js:816:30)
    at Promise.promise.promiseDispatch (C:\nvm\v8.11.3\node_modules\cordova\node_modules\cordova-lib\node_modules\q\q.js:749:13)
    at C:\nvm\v8.11.3\node_modules\cordova\node_modules\cordova-lib\node_modules\q\q.js:509:49
    at flush (C:\nvm\v8.11.3\node_modules\cordova\node_modules\cordova-lib\node_modules\q\q.js:108:17)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)
    at Function.Module.runMain (module.js:695:11)
(node:7428) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:7428) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

(Note that the fact that .git should probably not be handled as a platform is tracked in https://issues.apache.org/jira/projects/CB/issues/CB-14231 and not part of this issue/PR)

@raphinesse
Copy link
Contributor

Looks good to me. You need to update some tests to check for the new messages, though.

@brodybits
Copy link
Contributor

If I would do the following commands on a new Cordova project using cordova@8, npm@6.2.0 (node@10.7.0) on my mac:

  • cordova plugin add cordova-sqlite-storage
  • cordova platform add ios
  • cordova platform add browser
  • cordova platform add android

then remove cordova-sqlite-storage using the following command: cordova plugin rm cordova-sqlite-storage, I get the following otuput, with no ugly messages:

Uninstalling cordova-sqlite-storage from android
Android Studio project detected
Uninstalling cordova-sqlite-storage from browser
js-module uninstall called : plugins/cordova-sqlite-storage/www/SQLitePlugin.js
Uninstalling cordova-sqlite-storage from ios
Removing "cordova-sqlite-storage"
Removing plugin cordova-sqlite-storage from config.xml file...
Removing cordova-sqlite-storage from package.json

I wonder if the difference is due to Windows vs mac, node/npm version, or how your project is setup?

Should I try the same thing on my Windows machine?

@janpio
Copy link
Member Author

janpio commented Jul 30, 2018

No need to do anything @brodybits, this is the result of another unrelated problem - it just surfaced the suboptimal error messages in this file.

(This is caused by a folder .git under /platforms because I have it under separate git version control for this project because of reasons. The CLI can't handle that very well as it understands this folder as a platform. I assume creating another folder manually under platforms might have the same effect. Issue for the actual problem is here: https://issues.apache.org/jira/projects/CB/issues/CB-14231)

Thanks @raphinesse, will do so tomorrow. Kinda (positively!) surprised this is covered by tests :)

@brodybits
Copy link
Contributor

Thanks @janpio for the response. Shouldn't that information be in the description, and shouldn't the JIRA number be in the title?

I gotta say that your use case sounds valid and interesting, not so surprised that CLI would have trouble with it. I think it would be nice to see a more clear description of your use case somewhere in JIRA and/or PR description.

@janpio
Copy link
Member Author

janpio commented Jul 30, 2018

Shouldn't that information be in the description, and shouldn't the JIRA number be in the title?

No, as this PR does nothing to solve the problem (which is described in the JIRA issue I linked to and to which a PR fixing that issue will refer to).

This other problem just showed me that the error message here in this file could be better, so I created a PR directly to improve the outputs.

(One could create a JIRA issue "cordova-lib/plugin/uninstall message are unclear" but that is just busy work we can skip imho - the PR here contains all the relevant information on what it does and why. [I updated the PR description a bit so it actually does this])

@janpio janpio merged commit 6019d92 into master Jul 31, 2018
@janpio janpio deleted the janpio-patch-1 branch July 31, 2018 09:23
@janpio
Copy link
Member Author

janpio commented Jul 31, 2018

Adapted the tests and improved the initial PR description for future reference (like in the release notes and blog post etc).

Thanks for the quick approval @raphinesse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants