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

WIP: HTTP API #2495

Closed
wants to merge 29 commits into from
Closed

WIP: HTTP API #2495

wants to merge 29 commits into from

Conversation

mlynch
Copy link
Contributor

@mlynch mlynch commented Feb 26, 2020

This is a work-in-progress attempt to build a native HTTP plugin for Capacitor to help developers avoid CORS issues.

The API is loosely modeled off of fetch() and axios, and has one core request method along with several cookie methods:

const { Http } = Capacitor.Plugins;
await Http.request({
  method: 'POST',
  data: {
    name: 'Max'
  },
  headers: {
    'Content-Type': 'application/json'
  }
});

await Http.setCookie({ url: 'http://google.com', key: 'language', value: 'en' })
await Http.getCookies({ url: 'http://google.com' })

Currently have basic JSON fetching and posting working on iOS and setting/getting cookies for URLs.

Tasks:

  • Support posting form data (urlencoded and multipart)
  • Support file transfers
  • Support cookies
  • Make sure headers are being sent
  • Add downloads directory support
  • Support fileDirectory for download on iOS
  • Make sure timeouts/etc are configurable
  • Add timeout support for iOS
  • Web support using fetch
  • Always return body text even if content type doesn't match
  • See if we can fix [IOS] Cookies problems in IOS #1373 with this

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 minor comment

Also, use CAPLog.print instead of print so it can be disabled by the user, unless you plan to remove them before merging.

Other than that, looks good to me.

example/src/pages/http/http.html Show resolved Hide resolved
@silviogutierrez
Copy link

This is great news! I posted in the other thread, but this may be helpful when you run into edge cases:

https://github.com/facebook/react-native/tree/master/Libraries/Network

I don't speak Swift, so I'm not sure if there are any parallels. But I'm sure they've dealt with a few quirks already.

The only think worth mentioning, with myself included, is that cookie support was another big selling point of native HTTP. I hope that's the case with this new plugin.

Thanks for doing this! Huge step forward for Capacitor.

@mlynch
Copy link
Contributor Author

mlynch commented Feb 29, 2020

Thanks @silviogutierrez just added Cookie support. From what I gather from your link and the advanced HTTP plugin, being able to set a cookie for a URL (future requests for this URL will then send the cookie) and getting all cookies for a URL is sufficient?

@sean-perkins
Copy link

@mlynch I didn't realize that my team had this package private: https://github.com/TeamHive/capacitor-http

Unsure if it'll be of any help or not, but figured I'd share as it's timely 👍

@silviogutierrez
Copy link

@mlynch : that's awesome! Thanks.

I don't specifically set cookies for URLs directly. Rather, I just call API endpoints and they set cookies and get stored automagically. That's how the current native plugin does it.

My own dream would be to make everything transparent, but then it wouldn't really parallel fetch and other systems. That is, Let me decide what cookies to send with every request, just as I do headers, params, etc. But then things get wonky with expirations, honoring server overrides, etc.

So I think as long as its close to the fetch spec, all is well.

I really hope, like Fetch, this supports sending json and form/multipart data. Not all endpoints receive JSON after all.

Here is my shoddy abstraction around the current native plugin to make it closer to fetch. The interesting parts are around handling where the Cordova plugin triggers an exception whereas fetch does not. Hopefully it helps:

// I use a special POST_FORM type literal to make mine auto serialize form data, but ignore that if you want.

try {
    const headers = {/*CORDOVA PLUGIN FIZZLES OUT WITH UNDEFINED VALUES FOR HEADERS, FETCH DOES NOT */};
    const url = ''; // build URL manually injecting GET params so that it's closer to fetch. Can provide code.

    const response = await HTTP.sendRequest(url, {
        method:
            method === "POST_FORM" ? "post" : (method.toLowerCase() as "get"),
        data: "payload" in request ? request.payload : {},
        serializer: method === "POST_FORM" ? "urlencoded" : "json",
        headers,
    });
    const data =
        response.headers["content-type"] === "application/json"
            ? JSON.parse(response.data)
            : response.data;
    const responseHeaders = new Headers(response.headers);

    return {
        data,
        headers: responseHeaders,
    };
} catch (error) {
    const headers = new Headers(error.headers ?? {});

    if (error.status == 400) {
        const data = JSON.parse(error.error);

        return {
            status: 400,
            errors: data,
            headers,
        };
    } else if (
        error.status == 302 ||
        error.status == 301 ||
        error.status == 403 ||
        error.status == 404
    ) {
        const headers = new Headers(error.headers);

        return {
            status: error.status,
            headers,
        };
    } else if (error.status == 401) {
        return {
            status: 401,
            headers,
        };
    }

    return {
        error: true,
        message: `${error}`,
        details: JSON.stringify(error),
    };
}

@mlynch
Copy link
Contributor Author

mlynch commented Mar 1, 2020

Anyone up to help test? So far on iOS this is working:

Get/send json using Http.request
Get/set cookies using Http.setCookie and Http.getCookies (don't automatically get set yet if set from the server, working on that)
Download and upload files using Http.downloadFile and Http.uploadFile

cc @silviogutierrez @sean-perkins

@silviogutierrez
Copy link

@mlynch : Happy to test. I rely on my login endpoint setting a cookie, so I'd need one of:

  1. You mention auto-set is not happening yet. Can the existing code at least read the cookie response header and set it using Http.setCookie? Basically, do it manually. I ask because some implementations hide this header.
  2. Or I can wait till auto set works.
  3. If that's a while away and you want testing now, I could probably make an endpoint that bumps the header into the response body too. Slightly stranger and bigger attack surface here, but maybe just for testing.

With any of the above I can give it a full rundown.

Thanks again for all this work!

@mlynch
Copy link
Contributor Author

mlynch commented Apr 14, 2020

@nguyenduclong-ict yes it will work in the browser during serve.

@CaptJakk got an example of what that would look like? Happy to have something like that in here but would want to make sure it's pretty generic and only uses core APIs

@ProofOfKeags
Copy link
Contributor

ProofOfKeags commented Apr 14, 2020

we would add optional parameters to the http request and delegate the socks initiation to the underlying java and swift libs. specifically using URLSessionConfiguration for swift and HttpURLConnection for java has an overloaded constructor that will take a socks proxy. Ideally we'd make it as an optional final parameter to the 'request' call.

For the record Socks5 is very generic, it is just a way to proxy tcp traffic through another party. We plan on using this for Tor specifically but there are use cases beyond that. There is also precedent for things like this in axios on nodejs etc. The way we would propose adding this to the http api is making an optional parameter to a request containing the proxy information that would default to no proxy if it was not supplied, this way it would not affect the API for any users who did not care about it.

@sta55en
Copy link

sta55en commented Apr 21, 2020

I'm starting a new project and having this would simplify our design a lot, since we need background mode network access and don't want to use the 'advanced' http plugin. I can defer us having to build the network access code for a couple of weeks. What do we think the ETA for this making it into a main Capacitor release is? I hate asking, but are we talking a couple of weeks or a couple of months? This will be a massive step in the right direction for Capacitor! Native web access would speed up so many things in Ionic apps.

@ProofOfKeags
Copy link
Contributor

We’ve had some success factoring this plugin into a standalone capacitor plugin for our project, so it seems to be stable for the use cases it covers. It is worth noting though that binary content types do not work currently.

@sta55en
Copy link

sta55en commented Apr 21, 2020

@ProofOfKeags Thanks for the info Keagan. Do you mean binary content types, as in file downloads aren't working?

@curthusting
Copy link

I would be happy to test if you are still looking for testers. My application relies heavily on our JSON Api as well as file downloads. What is the best way to integrate this branch with my existing ionic/capacitor app for testing? I am currently on the latest for both ionic and capacitor.

@sbannigan
Copy link
Contributor

@mlynch going to plug this in to our enterprise app that we are rebuilding from the ground up and provide feedback on anything we run into. Any update on setting cookies automatically?

@ProofOfKeags
Copy link
Contributor

@sta55en I'm referring to specifically "application/octet-stream". It is possible to hack the plugin to make it work but it would require some sort of utf8 expansion (b64/b16) in order to be able to marshal it back over the native <> js boundary

let task = URLSession.shared.downloadTask(with: url) { (downloadLocation, response, error) in
if error != nil {
CAPLog.print("Error on download file", downloadLocation, response, error)
call.reject("Error", error, [:])
Copy link
Contributor

Choose a reason for hiding this comment

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

@mlynch I had to update all instances of call.reject to call.error when passing the error as an argument. Looks like #2533 changed that functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing similar problems in PluginCall.java. Probably just upstream changes that broke some things in your branch.

@sbannigan
Copy link
Contributor

The current Android cookie implementation does not persist cookies when quitting the app. According to the CookieManager documentation, by default cookies are only stored in memory. Ideally cookies should persist to prevent users from getting logged out every time the app quits and to provide the same experience as iOS. I was able to implement this library for persistent cookies and it seems to solve the problem. Possibly a good starting point for a custom built-in implementation if you don't want to introduce an external dependency.

@mlynch
Copy link
Contributor Author

mlynch commented May 1, 2020

Thanks everyone for the feedback. Took a little break from this and we agreed we're going to make this one available as our first package outside of the core APIs to make it easier to test and because we're interested in moving in that direction (this plugin would still be core but would be installed separately).

I will look to incorporate some of the above modifications and update everyone here once that plugin is ready to test (which should be much easier w/ these changes)

@mlynch
Copy link
Contributor Author

mlynch commented May 1, 2020

Alright the new version is up on @capacitor/http using the @next tag. I'm having some troubles with my publish script but it looks like it went through.

Someone want to try it out?

npm install @capacitor/http@next

Make sure your @capacitor/ios and @capacitor/android are latest as well!

Moving development to the new repo here: https://github.com/ionic-team/capacitor-plugin-http

@mlynch
Copy link
Contributor Author

mlynch commented May 1, 2020

@sbannigan Thanks. I'm wondering if I should use the android.webkit.CookieManager instead which should also have the benefit of making those cookies available in the webview automatically https://developer.android.com/reference/android/webkit/CookieManager

@mlynch
Copy link
Contributor Author

mlynch commented May 1, 2020

@sbannigan Alright I moved to the android.webkit.CookieManager and it seems to be persisting well. Give it a try at @capacitor/http@next which should be 0.0.3-0

@arsewizz
Copy link

arsewizz commented May 1, 2020

It possible to run capacitor plugins with Electron ?

@nbeekman
Copy link

nbeekman commented May 4, 2020

@mlynch I've installed @capacitor/http@next and still don't see the app saving the cookie like it should in storage. On log in, I see the Set-Cookie from the API's response, but it never gets saved in cookie storage. This is in iOS.
Screen Shot 2020-05-04 at 11 34 53 AM
Screen Shot 2020-05-04 at 11 35 33 AM

@mlynch
Copy link
Contributor Author

mlynch commented May 4, 2020

@nbeekman if you call the getCookie method of the plugin, does it return it? I'll look into this case

@zerock54
Copy link

zerock54 commented May 5, 2020

@mlynch I've just tested it and it works fine if I set a cookie with setCookie and then get it with getCookies !
We can't see it in the Safari storage but it's there

@nbeekman
Copy link

nbeekman commented May 5, 2020

@zerock54 so you still have to manually set/getCookie with the plugin?

@mlynch
Copy link
Contributor Author

mlynch commented May 6, 2020

Well it sounds like people would expect the cookie to actually get set in the webview as well, correct? It likely doesn't do that yet

@3adeling
Copy link

3adeling commented May 8, 2020

I am looking forward for this. I am suffering using HttpClient, fetch and cordova advanced http.

What about the session cookie? Do I have to call it manually or it will be attached with each request after I login in?

@muuvmuuv
Copy link

We are currently trying to make SSL pinning work but because that does not work with Angular's httpClient, I was looking for a native plugin and (yay!) someone is working on a Capacitor equivalent of cordova-plugin-advanced-http. Thank you in first place for the effort!

So will the WIP also include enabling SSL pinning? See; https://github.com/silkimen/cordova-plugin-advanced-http#setservertrustmode

@mlynch
Copy link
Contributor Author

mlynch commented May 28, 2020

Hey all, just an update: this plugin has moved to the (new) Capacitor Community org. More info about this org coming soon, but for now the plugin can be found here: https://github.com/capacitor-community/http

@mlynch
Copy link
Contributor Author

mlynch commented May 29, 2020

Hi everyone, just published the first non-prerelease of @capacitor-community/http! I'm happy with where it is for a first release. However, there is certainly more work to be done. Please join me over at the official repo for testing/feedback/issues: https://github.com/capacitor-community/http

And if you want to start using this plugin right now, you can install it from npm: https://www.npmjs.com/package/@capacitor-community/http

Thanks so much to everyone for the help and feedback during this process!

@mlynch mlynch closed this May 29, 2020
@silviogutierrez
Copy link

@mlynch : thanks so much for your work on this.

One suggestion: I cannot, for any reason, think why this wouldn't be built into core. What's the advantage of making this a community plugin? My take is that this should be built into Capacitor and very much core functionality. There's a big opportunity for Capacitor to have "first class" HTTP support with cookies/sans-CORS.

This is just my opinion, and it's heavily influenced by the "batteries included" philosophy of Python/Django, that I find sorely lacking in the NodeJS world. Just install Capacitor, and get to work with everything you (reasonably) need.

Thanks again for putting it together.

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.

[IOS] Cookies problems in IOS