Skip to content

chore: upgrade to stencil 3#118

Merged
sean-perkins merged 4 commits intomasterfrom
sp/upgrade-stencil
Jul 5, 2023
Merged

chore: upgrade to stencil 3#118
sean-perkins merged 4 commits intomasterfrom
sp/upgrade-stencil

Conversation

@sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Jun 21, 2023

Upgrades the Stencil project to v3, resolving the problem with consuming in a Vite project.

Resolves #109

This PR is in favor of #107.

@@ -4,136 +4,135 @@
* This is an autogenerated file created by the Stencil compiler.
Copy link
Contributor Author

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.

tag: 'pwa-camera',
styleUrl: 'camera.css',
assetsDir: 'icons',
assetsDirs: ['icons'],
Copy link
Contributor Author

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)

}

componentDidUnload() {
disconnectedCallback() {
Copy link
Contributor Author

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).


async componentDidLoad() {
if (this.isServer) {
if (Build.isServer) {
Copy link
Contributor Author

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: stenciljs/core@56d94f3

@@ -28,8 +28,8 @@
},
"homepage": "https://github.com/ionic-team/ionic-pwa-elements",
"devDependencies": {
Copy link
Contributor Author

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.

@sean-perkins sean-perkins marked this pull request as ready for review June 21, 2023 04:13
@sean-perkins sean-perkins requested a review from liamdebeasi June 21, 2023 04:14
Copy link

@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.

The changes look good, but is there a way I can test this in a Vite project?

Copy link

@rwaskiewicz rwaskiewicz left a 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

@sean-perkins
Copy link
Contributor Author

@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.

@liamdebeasi
Copy link

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.

@sean-perkins
Copy link
Contributor Author

@liamdebeasi I checked against the reproduction provided in: #109 and everything is working as expected 👍

@sean-perkins sean-perkins requested a review from liamdebeasi July 5, 2023 17:30
Copy link

@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 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.

@sean-perkins sean-perkins merged commit 5f3cde4 into master Jul 5, 2023
@sean-perkins sean-perkins deleted the sp/upgrade-stencil branch July 5, 2023 19:51
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.

Unable to load dynamic imports with vite

3 participants