-
Notifications
You must be signed in to change notification settings - Fork 51
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
chore: upgrade to stencil 3 #118
Conversation
@@ -4,136 +4,135 @@ | |||
* This is an autogenerated file created by the Stencil compiler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is autogenerated from stencil build
.
@@ -9,14 +9,12 @@ declare var window: any; | |||
@Component({ | |||
tag: 'pwa-camera', | |||
styleUrl: 'camera.css', | |||
assetsDir: 'icons', | |||
assetsDirs: ['icons'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assetsDir
was removed in Stencil v3 and replaced with assetsDirs
(source)
@@ -75,7 +73,7 @@ export class CameraPWA { | |||
await this.initCamera(); | |||
} | |||
|
|||
componentDidUnload() { | |||
disconnectedCallback() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
componentDidUnload
was removed in favor of disconnectedCallback
in Stencil v2 (source).
@@ -58,7 +56,7 @@ export class CameraPWA { | |||
flashMode: FlashMode = 'off'; | |||
|
|||
async componentDidLoad() { | |||
if (this.isServer) { | |||
if (Build.isServer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This context
API option was removed from Stencil in favor of !Build.isBrowser
(source).
Later, a new build conditional called Build.isServer
was introduced: ionic-team/stencil@56d94f3
@@ -28,8 +28,8 @@ | |||
}, | |||
"homepage": "https://github.com/ionic-team/ionic-pwa-elements", | |||
"devDependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped all dev dependencies to the latest available versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good, but is there a way I can test this in a Vite project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look reasonable to me 👍
I didn't test this, so this is probably best merged after someone (who reviewed) does that. LMK if you'd like a hand with that @liamdebeasi
@liamdebeasi are you wanting to test it against the Ionic React vite blank project for example? We don't have dev-builds configured for this repo, so we'd need to locally build/pack and install it into a consuming project. |
Yeah I wasn't sure if there was a sample app that the original bug was tested in? I could build the project and install it there to verify. |
@liamdebeasi I checked against the reproduction provided in: #109 and everything is working as expected 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the camera flow in https://github.com/DinosaurDad/capacitor-pwa-camera-vite using these changes, and I receive no dynamic import errors. I also did not receive any errors related to the Stencil upgrade.
Upgrades the Stencil project to v3, resolving the problem with consuming in a Vite project.
Resolves #109
This PR is in favor of #107.