-
Notifications
You must be signed in to change notification settings - Fork 1.6k
chore!: bump 9.0.0-dev and cordova-ios to 7.0.0 #939
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
Conversation
- cordova-ios 7 supports minimum iOS 11 which should be enough
- Version 8.0.1 is not released yet. The current version of the master is 8.0.1-dev and must be used as dependency version
|
You also need to update the engines listed in plugin.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my opinion, but I think this PR should be done a little differently.
Current PR Changes:
"8.0.0": {
"cordova-android": ">=12.0.0",
"cordova-ios": ">=5.1.0",
"cordova": ">=9.0.0"
},
"8.0.1-dev": {
"cordova-android": ">=12.0.0",
"cordova-ios": ">=7.0.0",
"cordova": ">=10.0.0"
},
Version 8.0.0 of the plugin already includes requirements and adding breaking changes should not be introduced in a patch or minor release.
We also never pinned -dev versions before here. I'm not fully sure how that behavior will play out.
I would recommend the following:
- Update the PR title to something like:
chore!: bump 9.0.0-dev - Update the package version in
package.json,plugin.xmland any other necessary files to9.0.0-dev. - Update the existing
9.0.0version (which currently lists"cordova": ">100") to10.0.0. - Replace the
8.0.1-deventry, from this PR, with9.0.0, without the-devsuffix. - Ensure that
plugin.xmlincludes the same requirements. - Run
npm installafter making these changes so thatpackage-lock.jsonis updated correctly.
IMO, I think we can use this PR to prepare for the 9.0.0 major development and future major release.
Regarding the plugin.xml documentation for the engines tag, it says:
Since this plugin supports a Cordova version newer than 6.1.0, can I remove the |
|
@erisu I can do this with the Regarding point 4:
When I set the version for this plugin in |
|
If I understand the plugin installation logic correctly, only the At least, this is my understanding. There might also be different behavior if |
|
Understand, do you know why there is a special field |
|
The actual requirements that are enforced are as defined in the plugin.xml file The idea with the list of cordovaDependencies in package.json is that if you did The platform versions should not be listed as |
But do I understand wrong, that this can be replaced by
I understand. If have for e.g.
I understand and makes sense. Maybe peerDependencies could be used for it along with peerDependenciesMeta. Maybe |
|
I recommend reading through https://github.com/apache/cordova-lib/blob/d2349bd6b4237ad512531e3b0ea2e6ab27cbf680/src/cordova/plugin/add.js#L311-L332 for background on the cordovaDependencies stuff. It's specifically not meant to be treated as dependencies of the plugin, but rather as requirements for the cordova tooling at plugin install time. |
|
Thank you. I didn't see that the |
|
|
|
I think to summarize what @dpogue was explaining: When a plugin is installed, the values defined in Following his example: When the command Once the version is decided and tries to run, it will run one more test against the values defined in The issue arises when In the case of installing from a local directory or repo, I think it skips this process. Using npm's properties— |
|
Thank you @erisu, for explaining everything. So the documentation is wrong about the |
- Removed wrong cordovaDependency for `5.0.4-dev`. `cordovaDependencies` matches only against npm released versions.
dpogue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now
|
@erisu I did everything like you mentioned in #939 (review). Also I removed the wrong dependency entry for |
I'm glad 😃 |
I do not see any statements indicating that the
If anything, I would suggest updating it to:
This wording uses phrases like "include" and "in addition to the Also, in the Specifying Cordova Dependencies documentation, there is this statement:
I would not change this. It says "intended to" which does not imply deprecation. It simply describes the long-term goal, which has not yet been completed. This suggestion is just my opinion... |
|
I wonder if a plugin will work, if |
5.0.4-dev.cordovaDependenciesmatches only against npm released versions.Platforms affected
Motivation and Context
I wanted to fix the deprecation warnings for this plugin and noticed, that this plugin still supports iOS 8 because cordova-ios was set to 5.1.0. I set cordova-ios to 7.0.0, so the plugin minimum supports iOS 11 which will help to fix the deprecations.
Description
Testing
Checklist
(platform)if this change only applies to one platform (e.g.(android))