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

Conversation

Projects
None yet
7 participants
@Salakar
Copy link
Collaborator

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 added some commits Nov 21, 2018

@Salakar Salakar changed the title [WIP] [feat] Add `enableCache` prop [feat] Add `enableCache` prop Nov 21, 2018

@Salakar

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 21, 2018

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

@thedusty99

This comment has been minimized.

Copy link

commented Nov 28, 2018

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

@Titozzz

This comment has been minimized.

Copy link
Collaborator

commented Nov 28, 2018

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

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2019

@Salakar Can you resolve the conflicts on this?

@munsifhayat

This comment has been minimized.

Copy link

commented Jan 21, 2019

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

@Titozzz

This comment has been minimized.

Copy link
Collaborator

commented Jan 21, 2019

@munsifhayat did you test this PR works for you ?

@munsifhayat

This comment has been minimized.

Copy link

commented Jan 21, 2019

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

@Titozzz

This comment has been minimized.

Copy link
Collaborator

commented Jan 21, 2019

Keep me updated

@jerolimov

This comment has been minimized.

Copy link
Contributor

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:

<NativeWebView
ref={this.webViewRef}
key="webViewKey"
style={webViewStyles}
source={resolveAssetSource(source)}
injectedJavaScript={this.props.injectedJavaScript}
bounces={this.props.bounces}
scrollEnabled={this.props.scrollEnabled}
pagingEnabled={this.props.pagingEnabled}
decelerationRate={decelerationRate}
contentInset={this.props.contentInset}

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 21, 2019

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

Titozzz added some commits Jan 22, 2019

wip
@Titozzz

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 23, 2019

LGTM 👌

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2019

Let's ship it!

@jerolimov

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Jan 29, 2019

Its working perfectly fine.

@Titozzz Titozzz merged commit 83ce79f into react-native-community:master Jan 30, 2019

2 checks passed

ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: tests Your tests passed on CircleCI!
Details

Titozzz pushed a commit that referenced this pull request Jan 30, 2019

chore(release): 4.0.0 [skip ci]
# [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

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2019

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@NeverFearTheasHere

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.