Skip to content

Allow source to be a required module#172

Merged
insraq merged 1 commit intoCRAlpha:masterfrom
Calyhre:fix/webview-source-require
Jul 24, 2018
Merged

Allow source to be a required module#172
insraq merged 1 commit intoCRAlpha:masterfrom
Calyhre:fix/webview-source-require

Conversation

@Calyhre
Copy link
Copy Markdown
Contributor

@Calyhre Calyhre commented Jul 24, 2018

React-native's webview allow html files to be directly required.

<WKWebView source={require('./index.html')} />

The fix is pretty straighforward, PropTypes already allow a number as source, managed by React-native internal packager. The only difference come from the initialisation of the source variable.
For obvious reasons, sendCookies and userAgent are being ignored.

Fixes #54

@insraq
Copy link
Copy Markdown
Contributor

insraq commented Jul 24, 2018

Thanks for the PR.

@insraq insraq merged commit 5cc3ebc into CRAlpha:master Jul 24, 2018
@Calyhre Calyhre deleted the fix/webview-source-require branch July 24, 2018 11:53
@DimitryDushkin
Copy link
Copy Markdown
Contributor

DimitryDushkin commented Aug 13, 2018

It broke normal source property as object with error

You attempted to set the key `sendCookies` with the value `undefined` on an object that is meant to be immutable and has been frozen.

It is not allowed to assign anything to this.props. On what react version did you test it? I have React 16.4.2.

It was Object.assign({},... which creates new object and it was right.
See https://github.com/CRAlpha/react-native-wkwebview/pull/172/files#r209639089

Comment thread WKWebView.ios.js
});
let source = this.props.source || {};
if (typeof source == 'object') {
source.sendCookies = this.props.sendCookies;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

source is still referencing to this.props.source

Copy link
Copy Markdown
Contributor Author

@Calyhre Calyhre Aug 13, 2018

Choose a reason for hiding this comment

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

Could be fixed by setting source like this I guess:

let source = { ...this.props.source };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't it break scenario with source={require('...')}?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed.
That's probably the if condition that fails, so how about a check like this instead:

if (typeof source == 'object' && source.constructor == Object) { //...

Copy link
Copy Markdown
Contributor

@DimitryDushkin DimitryDushkin Aug 13, 2018

Choose a reason for hiding this comment

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

require() returns number in react-native world. I think the fix might be like

let source = this.props.source;
if (this.props.source && typeof this.props.source === 'object') {
    source = Object.assign({}, this.props.source, {
        sendCookies: this.props.sendCookies,
        customUserAgent: this.props.customUserAgent || this.props.userAgent
    });

    if (this.props.html) {
        source.html = this.props.html;
    } else if (this.props.url) {
        source.uri = this.props.url;
    }
}

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.

Using require('...') as source

3 participants