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

Add shared process pool so cookies and localStorage are shared across webviews in iOS #138

Merged
merged 8 commits into from
Jan 7, 2019

Conversation

kylemantesso
Copy link
Contributor

Add a shared process pool in iOS to allow shared cookies and localStorage between webviews. This brings iOS into line with expected behaviour in Android.

@lewnelson
Copy link
Contributor

👏

@jamonholmgren
Copy link
Member

So this is awesome and we actually need it on one of our projects at Infinite Red; thanks for the PR @kylemantesso !

A couple questions, though:

  1. Given this will change current behavior and technically be a breaking change, should this be opt-in? Perhaps we should use a prop, like:
<WebView
  source={{ uri: "https://infinite.red/react-native" }}
  sharedProcess={true}
/>

This could result in some pretty subtle bugs if someone was relying on the previous behavior.

At the very least, we should introduce as a breaking change but allow opting out with sharedProcess={false}.

  1. Should we allow multiple (named) process pools?

I'd lean toward "no", but just putting it out there.

<WebView
  source={{uri: "https://infinite.red/ignite"}}
  processPool="MAIN"
/>

We could then track each process pool in a dict in native code.

Again, this is probably not necessary, but just putting it out there.

Thanks again!

@kylemantesso
Copy link
Contributor Author

Thanks @jamonholmgren! We also need this for a project, so keen to get it in!

I have refactored to make shared process pool optional, as per point 1 above. It will be a breaking change, but can be opted out by passing useSharedProcessPool={false} as a prop.

At this point we don't have a requirement for multiple process pools, but given the refactor this should make it easier to add later if it is required.

@felixmeziere
Copy link

Can we merge this?

@Titozzz
Copy link
Collaborator

Titozzz commented Nov 23, 2018

@felixmeziere did you test this PR ?

@Titozzz Titozzz added Maintainer: need testing Code looks good, can someone help us and test this ? and removed User: question Further information is requested labels Nov 23, 2018
@felixmeziere
Copy link

@Titozzz about to implement it on my project, is there a defined set of steps to test every feature?

@Titozzz
Copy link
Collaborator

Titozzz commented Nov 25, 2018

Sadly not yet 😔 but we will need one

@kylemantesso
Copy link
Contributor Author

Anything I can do to get this merged? What kind of testing do we need to perform?

@jamonholmgren
Copy link
Member

@kylemantesso @felixmeziere Apologies, I was out most of last week due to the holiday.

Have both of you tested this in your projects? If so, I'd say it's good to merge.

@Titozzz
Copy link
Collaborator

Titozzz commented Nov 27, 2018

Hello, sorry about this, I've been working on a huge refactoring of the code, hopefully it will land soon, and I can get back to PRs. 😄

@kylemantesso
Copy link
Contributor Author

@jamonholmgren Yes we are currently using it in our project for authentication cookie and local storage and it is working well.

@Titozzz
Copy link
Collaborator

Titozzz commented Nov 29, 2018

Should this be released as breaking since the default behaviour changed?

@tigo0 tigo0 mentioned this pull request Dec 13, 2018
@tigo0
Copy link

tigo0 commented Dec 14, 2018

This works just fine in our project. Thanks!

@jamonholmgren
Copy link
Member

@kylemantesso Can you resolve the conflicts? After that, I'd say we merge as breaking change. cc @Titozzz .

@jamonholmgren jamonholmgren added Maintainer: ready to publish This PR has been reviewed and is ready to be published and removed Maintainer: need testing Code looks good, can someone help us and test this ? labels Dec 21, 2018
…t-native-webview

# Conflicts:
#	ios/RNCWKWebView.m
#	js/WebView.ios.js
@kylemantesso
Copy link
Contributor Author

@jamonholmgren @Titozzz have merged changes and have tested, confirmed working to share cookies/localStorage in iOS. Can we please merge this as a breaking change?

@Titozzz Titozzz merged commit afadc62 into react-native-webview:master Jan 7, 2019
Titozzz pushed a commit that referenced this pull request Jan 7, 2019
# [3.0.0](v2.15.0...v3.0.0) (2019-01-07)

### Features

* **WKWebview:** Add shared process pool so cookies and localStorage are shared across webviews in iOS ([#138](#138)) ([afadc62](afadc62)), closes [#68](#68)

### BREAKING CHANGES

* **WKWebview:** useSharedProcessPool prop is set to true by default. If you want the old behavior, please use useSharedProcessPool={false}
@Titozzz
Copy link
Collaborator

Titozzz commented Jan 7, 2019

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

phuongwd pushed a commit to phuongwd/react-native-webview that referenced this pull request Apr 29, 2020
…are shared across webviews in iOS (react-native-webview#138)

* fix(WKWebview): [iOS] Add shared process pool so cookies and localStorage are shared across webviews (react-native-webview#68)

* Add optional shared process pool

BREAKING CHANGE: useSharedProcessPool prop is set to true by default. If you want the old behavior, please use useSharedProcessPool={false}
phuongwd pushed a commit to phuongwd/react-native-webview that referenced this pull request Apr 29, 2020
# [3.0.0](react-native-webview/react-native-webview@v2.15.0...v3.0.0) (2019-01-07)

### Features

* **WKWebview:** Add shared process pool so cookies and localStorage are shared across webviews in iOS ([react-native-webview#138](react-native-webview#138)) ([afadc62](react-native-webview@afadc62)), closes [react-native-webview#68](react-native-webview#68)

### BREAKING CHANGES

* **WKWebview:** useSharedProcessPool prop is set to true by default. If you want the old behavior, please use useSharedProcessPool={false}
@stan-sack
Copy link

@kylemantesso does localStorage survive an app restart on iOS. I'm noticing that it's cleared every time I reload my app in Expo Go. Im not having much luck finding the behaviour documented anywhere

jaynilson added a commit to jaynilson/reactNative_service_USA that referenced this pull request Sep 11, 2024
# [3.0.0](react-native-webview/react-native-webview@v2.15.0...v3.0.0) (2019-01-07)

### Features

* **WKWebview:** Add shared process pool so cookies and localStorage are shared across webviews in iOS ([#138](react-native-webview/react-native-webview#138)) ([afadc62](react-native-webview/react-native-webview@afadc62)), closes [#68](react-native-webview/react-native-webview#68)

### BREAKING CHANGES

* **WKWebview:** useSharedProcessPool prop is set to true by default. If you want the old behavior, please use useSharedProcessPool={false}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintainer: ready to publish This PR has been reviewed and is ready to be published
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants