apache / cordova-browser

Apache Cordova
Apache License 2.0
170 stars 85 forks source link

Remove Q for Native Promise #78

Closed erisu closed 5 years ago

erisu commented 5 years ago

Platforms affected

browser

Motivation and Context

Remove Q Efforts: https://github.com/apache/cordova/issues/7

Description

This repo did not have the Q dependency defined explicit in package.json but was still using Q that would have been a sub-dependency of cordova-common.

This PR removes the only usage of Q.

Testing



### Checklist

- [x] I've run the tests to see all new and existing tests pass
- [ ] I added automated test coverage as appropriate for this change
- [ ] 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](https://help.github.com/articles/closing-issues-using-keywords/))
- [ ] I've updated the documentation if necessary
erisu commented 5 years ago

Warning

I want to point out that this update script never checked or verified the provided project path. If the path is incorrect but does exist, it will still wipe out the contents of that folder. So, do not accidentally put in the wrong folder or you might end up loosing something you did not intend to delete.

brodybits commented 5 years ago

I want to point out that this update script never checked or verified the provided project path. [...]

That does not sound so good. Unfortunately I cannot tell for sure what is the corresponding behavior on the other platforms.

I would favor raising one or more issues to track both the behavior and level of consistency between the supported platforms.

I think it should be OK to continue with this change now, and deal with that behavior whenever we can.

I cannot tell whether or not this may be a breaking change. In case of any doubt, I would favor targeting this change for the next major release.

And a nit that I do not favor unchecked items in the checklist. I personally do strikethrough on the items that are not needed, not sure if we have any consensus about this.

erisu commented 5 years ago

I also agree that this does not sound good. Fixing the behavior or discussing the removal of this process can be separate issue ticket, as you pointed out. It might be easier if users just remove and add back the platform.

I also agree that we can continue on as the purpose of this PR is achieved. The issue I raised would be out of scope from this ticket. It just happened to have been discovered while working on this.

Anyways, I feel that this is not a major change and could be a minor release. I did not remove dependencies as it never existed in the package.json.

Also, I believe these changes only affect non-CLI workflow. I didn't see update.js required in the CLI workflow.

raphinesse commented 5 years ago

Regarding fixing the update script: IIRC, cordova-android and cordova-ios do not even support update. If we want to provide an update mechanism, we should probably just implement a generic delete/create in the CLI and remove it from the platforms altogether.

erisu commented 5 years ago

@raphinesse I never tried the CLI route. What I tested was the non-CLI, Platform-centric, use cases. If the Platform-centric use cases were removed, then we could simplify and make a generic remove and add process.