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

fix(http): route get requests through custom handler #6818

Merged
merged 26 commits into from
Jan 30, 2024

Conversation

ItsChaceD
Copy link
Contributor

@ItsChaceD ItsChaceD commented Aug 16, 2023

Closes #6123
Closes #6126

This PR routes fetch/XHR request methods (when CapacitorHttp is enabled) that don't allow bodies (GET, OPTIONS, HEAD, TRACE) to native protocol handlers so the responses don't have to be encoded/decoded through the bridge.

Opened in favor of outdated #6540

Test app: https://github.com/ItsChaceD/capacitor-http-wasm-issue

@deving1995
Copy link

  • I need this fix desperately

@IT-MikeS IT-MikeS changed the base branch from main to 5.x August 23, 2023 15:36
@jcesarmobile jcesarmobile changed the base branch from 5.x to main August 24, 2023 17:53
ios/Capacitor/Capacitor/WebViewAssetHandler.swift Outdated Show resolved Hide resolved
ios/Capacitor/Capacitor/WebViewAssetHandler.swift Outdated Show resolved Hide resolved
core/native-bridge.ts Outdated Show resolved Hide resolved
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

can you resolve the conflicts?

.replacingOccurrences(of: CapacitorBridge.httpsInterceptorStartIdentifier, with: "")

// Only replace first occurrence of the scheme
if let range = targetUrl.range(of: self.serverUrl?.scheme ?? InstanceDescriptorDefaults.scheme) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let range = targetUrl.range(of: self.serverUrl?.scheme ?? InstanceDescriptorDefaults.scheme) {
if let range = targetUrl.range(of: localUrl.scheme ?? InstanceDescriptorDefaults.scheme) {

should replace the localUrl scheme, not the serverUrl scheme, because when using live reload serverUrl scheme is http, but proxy urls are capacitor (or the configured scheme)

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Added a few comments.

Also, I've noticed that the proxy urls look like this capacitor://localhost//, with a double / at the end. Is that intended? I've read that while double / are accepted by servers they can cause problems on the browsers.

ios/Capacitor/Capacitor/WebViewAssetHandler.swift Outdated Show resolved Hide resolved
core/native-bridge.ts Outdated Show resolved Hide resolved
@ItsChaceD
Copy link
Contributor Author

Added a few comments.

Also, I've noticed that the proxy urls look like this capacitor://localhost//, with a double / at the end. Is that intended? I've read that while double / are accepted by servers they can cause problems on the browsers.

It wasn't intentional. I didn't notice any side effects, but I changed it to be safe

@giralte-ionic giralte-ionic merged commit b853d06 into main Jan 30, 2024
6 checks passed
@giralte-ionic giralte-ionic deleted the fix/http-request-custom-scheme branch January 30, 2024 21:46
@colbymelvin
Copy link

Any idea if this fix will be included in an upcoming release sometime soon?

My org is in the middle of migrating a large scale Cordova application to Capacitor. As far as we can tell, this is the last issue blocking us. We download image blobs and store them in SQLite so we can display them without an internet connection.

jcesarmobile added a commit that referenced this pull request Feb 28, 2024
Co-authored-by: jcesarmobile <jcesarmobile@gmail.com>
Co-authored-by: Mark Anderson <mark@ionic.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants