breautek commented on code in PR #334:
URL:
https://github.com/apache/cordova-plugin-media/pull/334#discussion_r856240299
##########
plugin.xml:
##########
@@ -34,7 +34,7 @@
<engine name="cordova-android" version=">=6.3.0" />
</engines>
- <dependency id="cordova-plugin-file" version="^6.0.0" />
+ <dependency id="cordova-plugin-file" version=">=6.0.0" />
Review Comment:
I understand that there is no code changes in this plugin specifically that
requires 7.x or [email protected] but that isn't the only thing that
determines compatibility.
Changing this to `>=` is a breaking change as it will cause
cordova-android@9 users who use this plugin to automatically attempt to install
v7 of the file plugin instead of v6, which is not compatible.
I've tested several different cases:
<details>
<summary>9.x fresh install</summary>
```
cordova plugin add github:ath0mas/cordova-plugin-media\#patch-1
Installing "cordova-plugin-media" for android
Installing "cordova-plugin-file" for android
Plugin doesn't support this project's cordova-android version.
cordova-android: 9.1.0, failed version requirement: >=10.0.0
Skipping 'cordova-plugin-file' for android
Adding cordova-plugin-media to package.json
```
</details>
<details>
<summary>9.x media plugin upgrade with existing file plugin
installation</summary>
```
cordova plugin remove cordova-plugin-media
Uninstalling cordova-plugin-media from android
Removing "cordova-plugin-media"
Removing cordova-plugin-media from package.json
norman@norman-P15:~/test/test$ cordova plugin add
github:ath0mas/cordova-plugin-media\#patch-1
Installing "cordova-plugin-media" for android
Plugin dependency "[email protected]" already fetched, using that
version.
Dependent plugin "cordova-plugin-file" already installed on android.
Adding cordova-plugin-media to package.json
```
</details>
<details>
<summary>Existing 9.x project restoration (with existing file plugin
installation)</summary>
```
Using cordova-fetch for [email protected]
Adding android project...
Creating Cordova project for the Android platform:
Path: platforms/android
Package: io.cordova.hellocordova
Name: HelloCordova
Activity: MainActivity
Android target: android-29
Subproject Path: CordovaLib
Subproject Path: app
Android project created with [email protected]
Discovered plugin "cordova-plugin-file". Adding it to the project
Installing "cordova-plugin-file" for android
The Android Persistent storage location now defaults to "Internal". Please
check this plugin's README to see if your application needs any changes in its
config.xml.
If this is a new application no changes are required.
If this is an update to an existing application that did not specify an
"AndroidPersistentFileLocation" you may need to add:
"<preference name="AndroidPersistentFileLocation"
value="Compatibility" />"
to config.xml in order for the application to find previously stored files.
Adding cordova-plugin-file to package.json
Discovered plugin "cordova-plugin-media". Adding it to the project
Installing "cordova-plugin-media" for android
Plugin dependency "[email protected]" already fetched, using that
version.
Dependent plugin "cordova-plugin-file" already installed on android.
Adding cordova-plugin-media to package.json
```
</details>
<details>
<summary>Media plugin upgrade when file plugin wasn't explicitly added by
the user</summary>
```
cordova plugin remove cordova-plugin-media
Uninstalling 1 dependent plugins.
Uninstalling cordova-plugin-file from android
Uninstalling cordova-plugin-media from android
Removing "cordova-plugin-media"
Removing cordova-plugin-media from package.json
norman@norman-P15:~/test/test$ cordova plugin add
github:ath0mas/cordova-plugin-media\#patch-1
Installing "cordova-plugin-media" for android
Installing "cordova-plugin-file" for android
Plugin doesn't support this project's cordova-android version.
cordova-android: 9.1.0, failed version requirement: >=10.0.0
Skipping 'cordova-plugin-file' for android
Adding cordova-plugin-media to package.json
```
</details>
<details>
<summary>9.x restoration with a project without explciit file plugin
installation</summary>
```
Using cordova-fetch for cordova-android@9
Adding android project...
Creating Cordova project for the Android platform:
Path: platforms/android
Package: io.cordova.hellocordova
Name: HelloCordova
Activity: MainActivity
Android target: android-29
Subproject Path: CordovaLib
Subproject Path: app
Android project created with [email protected]
Discovered plugin "cordova-plugin-media". Adding it to the project
Installing "cordova-plugin-media" for android
Installing "cordova-plugin-file" for android
Plugin doesn't support this project's cordova-android version.
cordova-android: 9.1.0, failed version requirement: >=10.0.0
Skipping 'cordova-plugin-file' for android
Adding cordova-plugin-media to package.json
```
</details>
As you can see, in order for this to work as expected, the user will need to
explicitly install the file plugin themselves, which breaks the point of the
dependency link.
Additionally, using `>=` is not a good idea because we won't be ever
introduce new major changes without this plugin automatically trying to pull
those changes in, even if it's not compatible with that major version.
Automatically allowing new majors to be pulled in unchecked is never a good
idea.
So for the reasons provided, I agree with @erisu . I think regardless we
need to build new major release anyway, so it would be a good time to bump the
engine to 10.x, and the file plugin dependency should be bumped to 7.x.
I think all of this also applies to the media-capture PR as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
