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

Prepare a capacitor 5 compatible version #32

Merged
merged 13 commits into from
May 4, 2023

Conversation

ptmkenny
Copy link
Contributor

This PR is a follow-up to #30.

These are the changes required according to the plugin migration guide.

@RaphaelWoude
Copy link
Owner

Thank you for your contribution. Would you be able to fix the linting errors for Android?

@ptmkenny
Copy link
Contributor Author

Hmm... I just checked the error, but I don't understand what it wants. It may be an error with Capacitor itself?

@NePheus
Copy link

NePheus commented Apr 29, 2023

You missed some changes in your PR.
Have a look to https://capacitorjs.com/docs/next/updating/plugins/5-0
You have to remove the package from the AndroidManifest.xml and they updated the guide to use gradle 8.

@ptmkenny
Copy link
Contributor Author

ptmkenny commented May 1, 2023

@NePheus Thanks, I hadn't seen that the gradle version had been bumped again, and I had failed to remove the package attribute. It should be fixed now after the last two commits.

@NePheus
Copy link

NePheus commented May 1, 2023

I think the PR fails because JavaVersion.VERSION_11 is set in the compile options in the build.gradle. It must be version 17 after this migration.

@ptmkenny
Copy link
Contributor Author

ptmkenny commented May 1, 2023

@NePheus Thanks for this hint. Would you happen to know how to set version 17? I tried doing so in this commit but I still get an error in the automated tests. Honestly I don't know how to fix this so any insight would be much appreciated.

@NePheus
Copy link

NePheus commented May 1, 2023

Maybe you should have a look how capacitor implemented it in their plugins.
https://github.com/ionic-team/capacitor-plugins/blob/main/app/android/build.gradle

@dallastjames
Copy link

👋 Hello! I'm just following up from an issue over on the Cap repo. You can also checkout our plugin upgrade guide which should be updated for the latest gradle and java changes from the latest beta version here. We've also been working on an automated migration path for plugins (a simple npx command) to make this jump for you that will be available soon. Keep an eye out for release day and we'll have all the details for that!

@ptmkenny
Copy link
Contributor Author

ptmkenny commented May 2, 2023

Changes I made to get this working:

  • Added setup-java action to use version 17 instead of 11 (the GitHub ubuntu image defaults to 11)
  • Bumped the capacitor dependencies in package.json from 4 to next. This will need to be set back to 5 when capacitor 5 gets its official release.
  • Updated Github Actions from node v14 to v18 because v14 support ended two days ago.
  • Bumped the github action versions for checkout + setup-node from 2 to 3 to clean up the deprecation warnings.
  • Removed the test step from the GitHub workflow. This is because it seems the plugin doesn't define any tests, and after updating to v18, this step started failing.

@NePheus
Copy link

NePheus commented May 2, 2023

Great! But now you have many commits that would be pushed to develop. I would suggest you to rebase/sqash your commits, or otherwise create a new branch with only the commits that are needed. Thanks for your work :)

@ptmkenny
Copy link
Contributor Author

ptmkenny commented May 2, 2023

@NePheus Thanks, I can do that if necessary, but the commits can automatically be squashed with the "Squash and merge" button here on GitHub (or merge --squash).

@RaphaelWoude
Copy link
Owner

Will be looking at the PR very soon. Just saw that capacitor v5 also released. So this will be a good time to get another version out.

Copy link
Owner

@RaphaelWoude RaphaelWoude left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

For now we will keep the test step out of the pipeline as it's not used.
In the future this will be re-added as unit tests are added to the project.

Thank you for your contribution.

@RaphaelWoude RaphaelWoude merged commit 9fda36e into RaphaelWoude:main May 4, 2023
4 checks passed
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.

None yet

4 participants