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

Upgrade all dependencies and create a new build for iOS #1

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

madhums
Copy link
Member

@madhums madhums commented May 1, 2023

closes https://github.com/q-m/questionmark-web/issues/370

  • prepare for cordova latest
  • use es6 string literal and change LANDING_URL to checker.thequestionmark.org
  • update config to include access and support for iPad
  • update cordova-ios@6.3.0
  • fix regex output

@madhums madhums requested a review from wvengen May 1, 2023 09:55
Copy link
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, you made it work ... congratulations!!
Some notes and questions.

README.md Show resolved Hide resolved
config.xml Outdated Show resolved Hide resolved
config.xml Show resolved Hide resolved
www/js/index.js Outdated Show resolved Hide resolved
www/js/index.js Show resolved Hide resolved
www/js/index.js Show resolved Hide resolved
@wvengen
Copy link
Member

wvengen commented May 1, 2023

Regarding deployment: Is the app at-risk of being unpublished in the iOS app store? If yes, then we'd better get this published. If not, then let's focus on the Android app first, test and make it work, publish in Google Play, then publish the iOS app later.

@madhums madhums requested a review from wvengen May 1, 2023 10:42
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
config.xml Outdated Show resolved Hide resolved
www/js/index.js Show resolved Hide resolved
@madhums
Copy link
Member Author

madhums commented May 1, 2023

iOS Distribution Certificate (App Store)
If your Apple Developer Program membership is valid, your existing apps on the App Store won't be affected. However, you'll no longer be able to upload new apps or updates signed with the expired or revoked certificate to the App Store.

From https://developer.apple.com/support/certificates/

I don't think we are at any risk of app being unpublished.

@madhums madhums requested a review from wvengen May 1, 2023 11:03
Copy link
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!
Now I'm curious how Android is doing :)

package.json Outdated Show resolved Hide resolved
www/js/index.js Show resolved Hide resolved
Copy link
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super Madhu, that you managed to get it building again and upgraded Cordova!
One comment about setup, otherwise ready to merge :)

Now I'll have a look at Android, which unfortunately doesn't build yet. I think plugins need to be updated / replaced for that.

npm i
npx cordova requirements # check requirements for building the app
npx cordova platform add ios # only on iOS supported platform
npx cordova platform add android
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest

npm install
npx cordova platform add ios      # when building iOS app on Mac OS
npx cordova platform add android  # when building Android app
npx cordova requirements          # check build requirements
npx cordova build ios             # when building iOS app on Mac OS
npx cordova build android         # when building Android app

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, feel free to do so!

@wvengen
Copy link
Member

wvengen commented May 2, 2023

Also, I would really like to make sure everything is tested (and I can only do that on Android), like various ways of navigating, barcode scanning, network online/offline in different conditions.

@madhums
Copy link
Member Author

madhums commented May 2, 2023

Yes I am testing too and it seems like there's some issue with custom scheme and scanning doesn't work properly. 😞

@madhums
Copy link
Member Author

madhums commented May 2, 2023

The barcode scanner doesn't open and the error seems to be:

2023-05-02 10:53:49.639816+0200 Questionmark[18034:1103463] -canOpenURL: failed for URL: "app://mobile-scan?ret=%2Flookup%3Fapp%3DQuestionmark%26q%3D%7BCODE%7D" - error: "The operation couldn’t be completed. (OSStatus error -10814.)"
2023-05-02 10:54:19.159734+0200 Questionmark[18034:1103463] Could not signal service com.apple.WebKit.WebContent: 113: Could not find specified service

I've tried these config in Info.plist without much luck:

        <key>LSApplicationQueriesSchemes</key>
	<array>
		<string>app</string>
	</array>
	<key>NSAppTransportSecurity</key>
	<dict>
		<key>NSAllowsArbitraryLoads</key>
		<true/>
	</dict>

And have the same issue with opening mailto:* or tel:* links (Similar to apache/cordova-plugin-inappbrowser#830)

Related old PR, perhaps this needs to be merged in order to make it work? @wvengen apache/cordova-plugin-inappbrowser#274

@madhums
Copy link
Member Author

madhums commented May 2, 2023

Another note: many of the cordova plugins seem to be unmaintained and haven't been updated since 3-4 years. It may be the end of life for cordova. Something to consider for the future - to switch to a different technology like ReactNative.

@wvengen
Copy link
Member

wvengen commented May 2, 2023

I see that many of the current plugins have not been upgraded yet, I'd expect that to be important for a succesful upgrade.
E.g. the barcode reader plugin using the Google ML model hasn't been updated. We can revert to the ordinary barcode scanner.
Some plugins have not been updated in more than a year, but are very much part of the core ecosystem - and may not need an update in this timespan (like inappbrowser). One year of no updates can be perfectly healthy in certain (usually smaller sub-) projects that reached maturity. Not sure if that applies in this case, however.

Something to consider for the future - to switch to a different technology like ReactNative.

That is something we don't have capacity for, as it is basically creating a new front-end. Cordova, or any other embed-website-app-framework, would still be preferred. Ideally, we wouldn't need an app anymore, but use this website-as-app thing (but I don't think barcode scanning has made it yet to browsers - except a slow JS-based version using the webcam that doesn't cut it on lower-end devices at least).

@wvengen
Copy link
Member

wvengen commented May 2, 2023

Related old PR, perhaps this needs to be merged in order to make it work? @wvengen apache/cordova-plugin-inappbrowser#274

Bummer, too bad it didn't work out to get this merged.
Seems like it still may still be quite some effort to get the feature we need in Cordova, even on a custom branch.

Thing is, at the time postMessage was not reliable enough, so I used a custom protocol to signal from the inAppBrowser to the app. One workaround would be to navigate to a dummy page on scan, which the app will recognize and do the scanning. Gives a short empty screen.

I had hoped my Cordova changes would have been accepted by the time a new upgrade was due. But because my contribution was just before Cordova's WKWebView introduction, I didn't have the time to develop it there. Sad it didn't work out. Now we either need make these changes, and hope to get them upstream (a better chance, hopefully).

Or use a wholly different approach for the app, now this is becoming more time-consuming ... :( E.g. load within the app (not in an inAppBrowser - but that is a lot less safe with all these external scripts and embeds in the website), or use another website wrapping framework. It seems Cordova is fading indeed. Capacitor seems a similar alternative, and even mentions migration from Cordova. But it is still a lot of work.

Perhaps it makes sense to reconsider how much work we want to put in maintaining the app ...

@madhums
Copy link
Member Author

madhums commented May 3, 2023

Since the app is not at the risk of being unpublished any time soon, I'd say we can give capacitor a try in the coming time. ReactNative along side https://github.com/react-native-webview/react-native-webview is also an option to consider.

@wvengen
Copy link
Member

wvengen commented May 3, 2023

https://github.com/react-native-webview/react-native-webview

Ah, that's a good idea, now I understand you better :) It's a bit of overkill in terms of size (shipping all the RN code including its own JS runtime), but could be a good idea in terms of development speed - especially considering we have some RN experience.

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.

2 participants