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] Add cacheEnabled prop #152

Merged
merged 12 commits into from Jan 30, 2019
Merged

Conversation

Salakar
Copy link
Collaborator

@Salakar Salakar commented Nov 20, 2018

This PR 'fixes' #128

cc @Titozzz @jamonholmgren as per linked issue above. Let me know your thoughts and if anything needs changing. I've enabled it by default as discussed with @Titozzz on slack.

Checklist

  • Android support
  • iOS support
  • Flow Types updated
  • Reference Documentation updated

Release Notes

  • [BREAKING] [FEAT] Added a new enableCache cacheEnabled prop to toggle Android & iOS webview caching behavior. This change makes caching enabled by default when previously there was no caching behavior which may cause unexpected behaviour changes in your existing implementations.

@Salakar Salakar changed the title [feat] Add enableCache prop [WIP] [feat] Add enableCache prop Nov 20, 2018
@Salakar Salakar changed the title [WIP] [feat] Add enableCache prop [feat] Add enableCache prop Nov 21, 2018
@Salakar
Copy link
Collaborator Author

Salakar commented Nov 21, 2018

Updated - this should be good to go now, cc @Titozzz @jamonholmgren

@Titozzz Titozzz added the Maintainer: need testing Code looks good, can someone help us and test this ? label Nov 21, 2018
@ghost
Copy link

ghost commented Nov 28, 2018

Hey there! Thanks for your work! Is there anything stopping this PR from getting merged (besides the conflicts now)?

@Titozzz
Copy link
Collaborator

Titozzz commented Nov 28, 2018

Yeah, it needs testing, because when I tested it I did not feel the expected improvements

@jamonholmgren
Copy link
Member

@Salakar Can you resolve the conflicts on this?

@munsifhayat
Copy link

Any updates on this one ?
Need to have this 'WebCache' enabled property.

@Titozzz
Copy link
Collaborator

Titozzz commented Jan 21, 2019

@munsifhayat did you test this PR works for you ?

@munsifhayat
Copy link

@munsifhayat did you test this PR works for you ?
did't checked yet. Working on another task.

@Titozzz
Copy link
Collaborator

Titozzz commented Jan 21, 2019

Keep me updated

@jerolimov
Copy link
Contributor

jerolimov commented Jan 21, 2019

For my project I can confirm that this PR improves the caching and performance. Thanks for this PR. I think this is a great and important improvement. 👍

@Salakar @munsifhayat @Titozzz But unfortunately, in the WebView.ios.js its required to push the props to the <NativeWebView... in line 249, simiar to the other ...enabled props:

https://github.com/react-native-community/react-native-webview/blob/ac59610f55fbe909b2120dfef029ac4b33811b60/js/WebView.ios.js#L248-L258

Without this _enableCache is always false in RNCWKWebView.m. I expect that its similar on Android but doesn't debug it yet.

I would also recommend to rename enableCache to cacheEnabled or cachingEnabled (or something similar), to match all the other ...enabled props

@Titozzz
Copy link
Collaborator

Titozzz commented Jan 21, 2019

@jerolimov excellent catch about the props naming. We need to do that. 👍

@Titozzz
Copy link
Collaborator

Titozzz commented Jan 23, 2019

@Salakar @jerolimov I updated the PR according to the reviews above :)
What do you think ?

@Titozzz Titozzz changed the title [feat] Add enableCache prop [feat] Add cacheEnabled prop Jan 23, 2019
@Salakar
Copy link
Collaborator Author

Salakar commented Jan 23, 2019

LGTM 👌

@jamonholmgren
Copy link
Member

Let's ship it!

@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 Jan 23, 2019
@jerolimov
Copy link
Contributor

jerolimov commented Jan 25, 2019

@Titozzz Sorry for the delay. Thanks for applying the feedback. Sourcecode looks fine. I tested it also in a live project on iOS and it works as expected. 👍

(I could not verify this on Android yet, because I have other troubles with my Android build atm.)

@munsifhayat
Copy link

munsifhayat commented Jan 25, 2019

@jerolimov
From my side I installed it got an RCTWebview error which I solved by dragging that library into Xcode project.

Imported the Webivew as required and than :
<WebView
ref="webview"
source={{ uri: this.state.url }}
onLoadEnd={e => this.webViewLoaded()}
domStorageEnabled = {true}
javaScriptEnabled = {true}
style={styles.container}
cacheEnabled = {true}
/>

Rest are working fine but 'cacheEnable' could be verified once I have live build Soon. Let me know if anything need to change.
I am using this for installing :

"react-native-webview": "git+https://github.com/react-native-community/react-native-webview.git"

@jerolimov
Copy link
Contributor

jerolimov commented Jan 25, 2019

@munsifhayat With your linked version, you referes to the latest react-native-webview. This version does not include the cacheEnabled prop until this PR got merged.

You need to refer the branch which is requested to merge here. You can define this in your package.json this way:

  ...
  "react-native-webview": "react-native-webview@invertase/react-native-webview#master"
  ...

I install this with yarn (npm install should work similar):

yarn add "react-native-webview@invertase/react-native-webview#master"

You find the username and branch of the PR in the top-right corner of the PR:

screenshot 2019-01-25 at 10 58 42

Hope this helps.

@munsifhayat
Copy link

Its working perfectly fine.

@Titozzz Titozzz merged commit 83ce79f into react-native-webview:master Jan 30, 2019
Titozzz pushed a commit that referenced this pull request Jan 30, 2019
# [4.0.0](v3.2.2...v4.0.0) (2019-01-30)

### Features

* **iOS/Android:** Add `cacheEnabled` prop ([#152](#152)) ([83ce79f](83ce79f))

### BREAKING CHANGES

* **iOS/Android:** This change makes caching enabled by default when previously there was no caching behavior which may cause unexpected behaviour changes in your existing implementations.
@Titozzz
Copy link
Collaborator

Titozzz commented Jan 30, 2019

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@NeverFearTheasHere
Copy link
Contributor

I think this cacheEnabled prop is missing from the typescript prop type declarations (intypings/index.d.ts) so I've opened a PR for that: #335

phuongwd pushed a commit to phuongwd/react-native-webview that referenced this pull request Apr 29, 2020
Added a new cacheEnabled prop to toggle Android & iOS webview caching behavior.

BREAKING CHANGE:  This change makes caching enabled by default when previously there was no caching behavior which may cause unexpected behaviour changes in your existing implementations.
phuongwd pushed a commit to phuongwd/react-native-webview that referenced this pull request Apr 29, 2020
# [4.0.0](react-native-webview/react-native-webview@v3.2.2...v4.0.0) (2019-01-30)

### Features

* **iOS/Android:** Add `cacheEnabled` prop ([react-native-webview#152](react-native-webview#152)) ([83ce79f](react-native-webview@83ce79f))

### BREAKING CHANGES

* **iOS/Android:** This change makes caching enabled by default when previously there was no caching behavior which may cause unexpected behaviour changes in your existing implementations.
noproblem23 added a commit to noproblem23/react-native-webview that referenced this pull request Apr 18, 2023
# [4.0.0](react-native-webview/react-native-webview@v3.2.2...v4.0.0) (2019-01-30)

### Features

* **iOS/Android:** Add `cacheEnabled` prop ([#152](react-native-webview/react-native-webview#152)) ([83ce79f](react-native-webview/react-native-webview@83ce79f))

### BREAKING CHANGES

* **iOS/Android:** This change makes caching enabled by default when previously there was no caching behavior which may cause unexpected behaviour changes in your existing implementations.
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.

None yet

6 participants