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

refactor: use capacitor types for native plugins #27755

Merged
merged 16 commits into from
Jul 14, 2023
Merged

refactor: use capacitor types for native plugins #27755

merged 16 commits into from
Jul 14, 2023

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Jul 6, 2023

Issue number: Internal


What is the current behavior?

Ionic currently detects and uses Capacitor APIs for different plugins (haptics, status bar and keyboard). This implementation does not have type safety and can result in unexpected behaviors.

What is the new behavior?

  • Adds @capacitor/core, @capacitor/keyboard, @capacitor/haptics and @capacitor/status-bar as dev dependencies. These should only be used with import type { }.
  • Refactors the plugin usages to be typed against the plugin packages, while using a duplicate enum when needing a value. This allows us to not bundle the capacitor plugins with Ionic Framework.
  • Introduces a getCapacitor() function for interacting with the window.Capacitor object through a typed object.

How does it work?

The idea is we want the type safety from the Capacitor packages, without directly bundling that source code within Ionic Framework. This means we use the Capacitor deps where a type is needed, but clone any enums where a value is referenced. If a Capacitor dep changes the supported values, Typescript will fail to compile and that will signal to use to update our enum values to match any changes.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev-build: 7.1.2-dev.11688696027.1c4d4ad1

Tested against a demo app for some of the core behavior: https://github.com/sean-perkins/capacitor-ionic-plugins-demo

@stackblitz
Copy link

stackblitz bot commented Jul 6, 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 Jul 6, 2023
@sean-perkins sean-perkins marked this pull request as ready for review July 7, 2023 03:01
core/package.json Show resolved Hide resolved
core/src/utils/native/capacitor.ts Outdated Show resolved Hide resolved
core/src/utils/native/haptic.ts Show resolved Hide resolved
core/src/utils/native/haptic.ts Show resolved Hide resolved
core/src/utils/native/capacitor.ts Outdated Show resolved Hide resolved
core/src/utils/native/capacitor.ts Outdated Show resolved Hide resolved
core/src/utils/native/haptic.ts Outdated Show resolved Hide resolved
core/src/utils/native/haptic.ts Show resolved Hide resolved
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.

I'm going to test the dev build on my end too, but some final comments on the code.

core/src/utils/native/keyboard.ts Outdated Show resolved Hide resolved
core/src/utils/native/haptic.ts Show resolved Hide resolved
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.

Tested locally and it works great! Tested with the datetime wheel picker for haptics as well as when Capacitor (but not the plugins) are available. I left a couple comments, but this is good to merge once those are addressed.

@sean-perkins sean-perkins added this pull request to the merge queue Jul 14, 2023
Merged via the queue into main with commit 16c77cc Jul 14, 2023
45 checks passed
@sean-perkins sean-perkins deleted the sp/FW-4622 branch July 14, 2023 15:44
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.

2 participants