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

Fixes #272: messages.forEach is not a function #273

Merged
merged 6 commits into from
Sep 29, 2018

Conversation

fungilation
Copy link
Contributor

Fixes #272

This is more of a bandaid. Should investigate how is messages getting malformed

fungilation and others added 5 commits December 11, 2017 15:45
Fixes and tested working in RN 0.51
- fixes single quote escaping in json payload
This is more of a bandaid. Should investigate how is messages getting malformed
> multiple methods named 'goForward' found with mismatched result, parameter type or attributes
in RCTWebViewBridgeManager.m, line 89

Changes match other RCT_EXPORT_METHODs, like goBack above
@chansuke chansuke self-requested a review September 13, 2018 10:02
@chansuke
Copy link
Collaborator

@fungilation Thx.I will check

@fungilation
Copy link
Contributor Author

FYI, I've been committing multiple fixes to my fork, which all lumped into initial PR for fixing #272.

Copy link
Collaborator

@chansuke chansuke left a comment

Choose a reason for hiding this comment

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

Some of the conflict remains with the merged PR below but I fixed by hand, is that ok?
c73b2c4

Copy link
Contributor Author

@fungilation fungilation left a comment

Choose a reason for hiding this comment

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

@chansuke I can test this merged PR once it's merged to master, for any breakage in my app.

Thanks for merging this and other PRs!

@@ -14,7 +14,6 @@
#import <React/RCTUIManager.h>
// #import <React/RCTBridge.h> // in forked https://github.com/lefnire/react-native-webview-bridge

#import "RCTWebViewBridge.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, imports moved around. I can test once this changes are merged, for any breakage in my app.

WebViewBridgeManager
}
WebViewBridgeManager,
},
} = ReactNative;
var { PropTypes } = PropTypes;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line should be deleted, redundant with var PropTypes = require('prop-types'). Can be used as is.

Same thing in index.android.js

@@ -17,6 +17,7 @@ var React = require('react');
React.createClass = require('create-react-class');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line should be deleted, only createReactClass is used

@@ -18,6 +18,7 @@ var React = require('react');
React.createClass = require('create-react-class');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line should be deleted, only createReactClass is used

@@ -37,7 +37,7 @@ var {
WebViewBridgeManager
}
} = ReactNative;
var PropTypes = require('prop-types');
var { PropTypes } = PropTypes;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line should be deleted, redundant like in index.ios.js

@chansuke
Copy link
Collaborator

Ok,I'll merge this then.Thank you for your work.

@chansuke chansuke merged commit 326fdbf into alinz:master Sep 29, 2018
@fungilation
Copy link
Contributor Author

Cool. I see a npm bump too (after 2 years!)

But what about the 5 changes/cleanup I suggested in review above?

@chansuke
Copy link
Collaborator

@fungilation Thanks for the confirmation and your feedbacks.

Well, I'm planning to merge these changes/cleanups in v0.40.1 and will release in next week.

Thanks so much for your work.

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.

None yet

2 participants