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

feat(app/capacitor): upgrade to capacitor 2.0, fixes #5823 #6808

Merged
merged 4 commits into from
Jun 4, 2020

Conversation

nklayman
Copy link
Contributor

@nklayman nklayman commented Apr 13, 2020

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Documentation
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

This PR will use a new method to enable https support for capacitor. It is only available since version 1.5.1 of capacitor. In addition, new projects created will use capacitor 2.0. Other than the https issue, it will work with existing pre-2.0 capacitor projects. I can add in a check to use the old https method for pre-1.5.1 projects to mitigate the breaking change.

The PR fulfills these requirements:

  • It's submitted to the dev branch and not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on a Electron app
  • Any necessary documentation has been added or updated in the docs (for faster update click on "Suggest an edit on GitHub" at bottom of page) or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

Dev/build process stays the same. Have not testing for IOS, someone has to before this gets merged. Also need someone to test that native plugins still work with https enabled.

@J3m5
Copy link

J3m5 commented Apr 14, 2020

This is great, thank you!

You might want to change the hard coded path here to the real ones, because the path is only valid if the capacitor or cordova id has not been modified.
If it has, the patch will no be applied.

@nklayman
Copy link
Contributor Author

@J3m5 My latest commit should have fixed that issue, it uses a glob to automatically find the proper path. If you could test it out that would be awesome.

@J3m5
Copy link

J3m5 commented Apr 30, 2020

I'll try to test it today and keep you posted.

@IlCallo
Copy link
Member

IlCallo commented May 5, 2020

@J3m5 hey there! Did you find the time to test this?

@J3m5
Copy link

J3m5 commented May 5, 2020

Hey !
Unfortunately no, I've been quiet busy in the last few days.
I'll try tomorrow and let you know.

@J3m5
Copy link

J3m5 commented May 7, 2020

So, I've tested it, it handles the self signed certificate errors correctly but it still doesn't solve the problem of missing cordova and capacitor runtime in the index.html file that is served to the client.

Like I previously said in issue #6771, I don't think it is strictly related to the ssl errors, but it has more to do with two possible things:

  1. The file served with https enabled in not the one that is modified by capacitor ( with the cordova and capacitor scripts injected)

  2. The good file is served but the injection didn't happened

@J3m5
Copy link

J3m5 commented May 29, 2020

I think we could merge this and deal with the ssl error in another PR to resolve the issues caused by the capacitor upgrade problems.

@nklayman nklayman marked this pull request as ready for review May 29, 2020 19:20
@rstoenescu rstoenescu changed the base branch from dev to app-v2-wip June 4, 2020 17:18
@rstoenescu rstoenescu merged commit 7366b85 into quasarframework:app-v2-wip Jun 4, 2020
@rstoenescu
Copy link
Member

Merging, testing, and adapting to q/app v2.

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.

4 participants