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

fix(core): handle uncaught native keyboard exceptions #27514

Merged
merged 4 commits into from
May 24, 2023

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented May 19, 2023

Issue number: Resolves #27503


What is the current behavior?

Ionic Framework wraps the implementation around Capacitor's Keyboard plugin API, to provide additional functionality and behavior "automatically" in Ionic, when the plugin is installed.

Certain methods such as getResizeMode() are only available for certain platforms and will cause an error when running in the unsupported platform.

Ionic Framework does not check to see if that platform is active before calling potentially unsupported methods, which leads to an exception for scenarios such as this - calling getResizeMode() on Android.

What is the new behavior?

  • Handles the uncaught exception by returning undefined if the plugin method is unavailable.
  • Developers do not receive an uncaught exception on Android when using the Capacitor Keyboard plugin with Ionic Framework

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev-build: 7.0.8-dev.11684444351.1b1ab142 (outdated)

@stackblitz
Copy link

stackblitz bot commented May 19, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label May 19, 2023
@sean-perkins sean-perkins marked this pull request as ready for review May 19, 2023 14:41
@sean-perkins sean-perkins requested a review from a team as a code owner May 19, 2023 14:41
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

@sean-perkins
Copy link
Contributor Author

@liamdebeasi I'm going to add you as an additional reviewer since you wrote a lot of the code around this. Any concerns to this approach?

@@ -18,7 +20,9 @@ export const Keyboard = {
},
getResizeMode(): Promise<KeyboardResizeOptions | undefined> {
const engine = this.getEngine();
if (!engine || !engine.getResizeMode) {
if (!isPlatform('ios') || !engine?.getResizeMode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to check the UNIMPLEMENTED value from Capacitor instead of checking the platform?

So something like this:

try {
  return engine.getResizeMode();
} catch(e) {
  if (e.code === 'UNIMPLEMENTED') {
    return Promise.resolve(undefined); 
  }
  
  // throw the error unchanged if not an unimplemented error
  throw e;
}

My concern with the proposed approach is we are coupling our code to implementation details of the Capacitor plugin. Additionally, isPlatform relies on user agent sniffing which can be spoofed, so that check may not always be correct. Checking the unimplemented state lets us rely on Capacitor to tell us about functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh that's brilliant, I didn't think of that approach!

Yes, we can do something very similar to this. I ended up using a .catch on the original promise, so when it is chained in the implementation, it will resolve undefined. It'll swallow the error in the client runtime, but still indicate the unimplemented method invocation in the native bridge logs.

This is a slightly different net result that the original implementation, which doesn't log the console.error for the unimplemented method invocation (because it isn't executed), but solves the original problem of the uncaught error.

CleanShot 2023-05-23 at 23 14 12@2x

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

One comment, but otherwise looks good. Thanks for fixing this!

core/src/utils/native/keyboard.ts Outdated Show resolved Hide resolved
@sean-perkins sean-perkins changed the title fix(core): Keyboard resize mode is only called on iOS fix(core): Handle uncaught native keyboard exceptions May 24, 2023
@liamdebeasi liamdebeasi changed the title fix(core): Handle uncaught native keyboard exceptions fix(core): handle uncaught native keyboard exceptions May 24, 2023
@sean-perkins sean-perkins added this pull request to the merge queue May 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 24, 2023
@sean-perkins sean-perkins added this pull request to the merge queue May 24, 2023
Merged via the queue into main with commit 0e7359c May 24, 2023
45 checks passed
@sean-perkins sean-perkins deleted the sp/keyboard-resize-mode branch May 24, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Ionic call Keyboard.getResizeMode() on Android throws uncaught error
3 participants