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

Enable wk cookie store #171

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

pwallrich
Copy link

To improve the cookie handling a new option useWKCookieStore is introduced.
When using useWKCookieStore, the cookies from HTTPCookieStorage get copied to the webView’s WKHTTPCookieStorage.
The cookie Plugins https://github.com/joeferraro/react-native-cookies/ and https://github.com/shimohq/react-native-cookie do not support WKHTTPCookieStorage at the moment and use NSHTTPCookieStorage to save the cookies.
That's why the cookies are copied from the NSHTTPCookieStorage.
A workaround is used to initialize the WKHTTPCookieStorage correctly. The WebKit bug https://bugs.webkit.org/show_bug.cgi?id=185483 prevents the initialization.

@SteffenHummel
Copy link

We need this!

@jigfox jigfox force-pushed the enable-wk-cookie-store branch from 0e784be to 3eac1f5 Compare July 24, 2018 12:57
@AndiW1988
Copy link

That´s really cool! Thank you!

@michelalbers
Copy link

+1

@insraq
Copy link
Contributor

insraq commented Jul 28, 2018

The code looks good to me. However, I don't really have time to test it myself. Can anyone conduct some tests? If it works, I'd be happy to merge this change.

@BlackSword101
Copy link

can we use it with "react-native-cookies", i didn't understand how can we use this please can you give us an example thanks.

@pwallrich
Copy link
Author

You would use react-native-cookies, the way you normally use it. The only difference would be to use the WKWebViewwith useWKCookieStore={true}. So you'd initialise it like this:
<WKWebView useWKCookieStore={true} />
WKCookieStorage will then be initialised and all Cookies will be copied to WKHTTPCookieStorage.

@jigfox jigfox force-pushed the enable-wk-cookie-store branch from 8dff228 to 36c7c36 Compare August 15, 2018 08:33
@ashishmusale
Copy link
Contributor

ashishmusale commented Sep 7, 2018

This looks great and works really well. Is there a plan to merge this? If there is any testing required to make this ready to merge, I am happy to help.

[cDesc appendFormat:@"domain=%@;", [cookie domain]];
if ([cookie.path length] > 0)
[cDesc appendFormat:@"path=%@;", [cookie path]];
if (cookie.expiresDate != nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Its missing HttpOnly, secure and sessionOnly property. Is this something you would be able to add?

Copy link

Choose a reason for hiding this comment

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

for the HttpOnly: I can do that,
for the sessionOnly there ist no such thing, If there is no expire date than it should be a session cookie, otherwise it will be stored until the expire date

Copy link
Contributor

Choose a reason for hiding this comment

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

https://developer.apple.com/documentation/foundation/nshttpcookie/1392991-sessiononly?language=objc

I was referencing that boolean to indicate its a session only cookie

Copy link
Contributor

Choose a reason for hiding this comment

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

And Thanks for fixing this issue!

@jigfox jigfox force-pushed the enable-wk-cookie-store branch from 50a49a8 to 7f2e56d Compare October 1, 2018 12:54
@@ -512,6 +512,18 @@ - (void)scrollViewDidScroll:(UIScrollView *)scrollView

#pragma mark - WKNavigationDelegate methods

- (void)webView:(WKWebView *)webView decidePolicyForNavigationResponse:(WKNavigationResponse *)navigationResponse decisionHandler:(void (^)(WKNavigationResponsePolicy))decisionHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

there are duplicate methods with this same name, can you please fix this? This is breaking my build with compilation issue

@@ -678,20 +690,5 @@ - (void)webViewWebContentProcessDidTerminate:(WKWebView *)webView
RCTLogWarn(@"Webview Process Terminated");
}

- (void)webView:(WKWebView *)webView decidePolicyForNavigationResponse:(WKNavigationResponse *)navigationResponse decisionHandler:(void (^)(WKNavigationResponsePolicy))decisionHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick response. Appreciate it

robinheinze added a commit to infinitered/react-native-wkwebview that referenced this pull request Oct 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants