search

apache / cordova-ios

Apache Cordova iOS
https://cordova.apache.org/
Apache License 2.0
2.15k stars 987 forks source link

refactor!: drop platform binaries #1180

Closed erisu closed 2 years ago

erisu commented 2 years ago

Motivation and Context

This PR:

Description

Testing

Checklist

codecov-commenter commented 2 years ago

Codecov Report

Merging #1180 (381ed72) into master (a6044dd) will increase coverage by 0.07%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
+ Coverage   74.95%   75.03%   +0.07%     
==========================================
  Files          14       13       -1     
  Lines        1705     1650      -55     
==========================================
- Hits         1278     1238      -40     
+ Misses        427      412      -15
Impacted Files Coverage Δ
bin/templates/scripts/cordova/Api.js 70.58% <0.00%> (-1.72%) :arrow_down:
bin/templates/scripts/cordova/lib/build.js 44.09% <ø> (-8.03%) :arrow_down:
bin/templates/scripts/cordova/lib/run.js 22.22% <ø> (+2.59%) :arrow_up:
bin/templates/scripts/cordova/loggingHelper.js

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a6044dd...381ed72. Read the comment docs.

erisu commented 2 years ago

Other binaries & scripts that will need reviewing... Maybe another PR?

I believe some of these could be removed and/or merged into some JS file(s) and exported.

raphinesse commented 2 years ago

Other binaries & scripts that will need reviewing... Maybe another PR? ... I believe some of these could be removed and/or merged into some JS file(s) and exported.

The following bins are used outside of cordova-ios and should be left alone for now:

apple_ios_version
./lib/src/plugman/util/default-engines.js
43:            { platform: 'ios', scriptSrc: path.join(project_dir, 'cordova', 'apple_ios_version') },

apple_osx_version
./lib/src/plugman/util/default-engines.js
45:            { platform: 'ios', scriptSrc: path.join(project_dir, 'cordova', 'apple_osx_version') },

apple_xcode_version
./lib/src/plugman/util/default-engines.js
41:            { platform: 'ios', scriptSrc: path.join(project_dir, 'cordova', 'apple_xcode_version') },

list-devices
./lib/src/cordova/targets.js
35:    const caller = { script: 'list-devices' };
37:    const cmd = path.join(projectRoot, 'platforms', platform, 'cordova', 'lib', 'list-devices');

list-emulator-images
./lib/src/cordova/targets.js
42:    const caller = { script: 'list-emulator-images' };
44:    const cmd = path.join(projectRoot, 'platforms', platform, 'cordova', 'lib', 'list-emulator-images');

These are not used outside of cordova-ios and could be removed in this PR:

erisu commented 2 years ago

Yes the pre-commit hook can be removed. I didn't notice it and it does not get runned.

erisu commented 2 years ago

@raphinesse I will excluded cordova_plist_to_config_xml from the dropping for now as it is written in a log output.

https://github.com/apache/cordova-ios/blob/master/CordovaLib/Classes/Public/CDVViewController.m#L155

Alternative message will need to be decided and maybe we could keep the script in some folder for handy scripts but not release it with the packages.

raphinesse commented 2 years ago

@raphinesse I will excluded cordova_plist_to_config_xml from the dropping for now as it is written in a log output.

https://github.com/apache/cordova-ios/blob/master/CordovaLib/Classes/Public/CDVViewController.m#L155

Alternative message will need to be decided and maybe we could keep the script in some folder for handy scripts but not release it with the packages.

I think the script and log message can both be removed without replacement. It's a python script that is supposed to convert Projects to the Cordova 2.3 format: https://github.com/apache/cordova-ios/blob/a6044dd4a586cac602ab4b0a87d925b1dd60fbfd/bin/cordova_plist_to_config_xml#L21

But it might indeed be wise to do that in a separate PR and decide how to handle that whole if statement that is seemingly handling a very old legacy case.

raphinesse commented 2 years ago

Yet another thing I noticed: we should be able to remove all remaining *.bat files. As mentioned in https://github.com/apache/cordova-android/pull/1100:

We could remove all the *.bat counterparts though, since execa (which is used to run the binaries) also implements shebang-lookup on Windows. This will break compatibility to any CLI version <10, since we did not yet use execa then.

Should we do that as well, then?

erisu commented 2 years ago

Since we already removed it from the Android platform and released it, maybe it is safe to remove it from here as well?

I wonder if anyone has any concerns, even though it was already removed in Android... If we remove it and later it is seen as an issue, we can easily add back as a patch release.

raphinesse commented 2 years ago

I'd remove the bat files. That's my 2¢

erisu commented 2 years ago

OK, the three last bat files were removed: