-
Notifications
You must be signed in to change notification settings - Fork 110
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
0.7 support #1
0.7 support #1
Conversation
New packager in react-native 0.7 doesn't seem to like main scripts other than index.js
In 0.7, we should always use exports on require('react-native') instead of, for example, require('NativeModules')
Thanks for the kind words @mnylen. This PR is a good start for getting 0.7 support, but I think we should hold off on merging it until 0.7 is released. |
@@ -2,7 +2,7 @@ | |||
"name": "react-native-safari-view", | |||
"version": "0.1.0", | |||
"description": "A React Native wrapper for Safari View Controller", | |||
"main": "SafariViewMananger.ios.js", | |||
"main": "index.ios.js", | |||
"scripts": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks weird to me, since the repo doesn't have an index.ios.js
file.
After reading the issue you liked to, it seems more like a react-native bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has - I renamed SafariViewManager.ios.js in the PR as index.ios.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I missed that. 😁
I'd still like to wait for the outcome of facebook/react-native#1808 prior to renaming the ios.js file.
I'd rather the file names remain consistent with the way react-native new-library
structures libraries.
Yeah, I agree - I hope this pull request isn't even needed and the weird require behaviour gets fixed in react-native itself before 0.7 is released. Though, require('NativeModules') will still stay deprecated. |
Glad we're on the same page. (: If you want to cherry-pick the deprecation change into its own PR we could merge that in now. |
Just FYI, 0.71 was released 9 days ago so wondering if this can be merged 😄 |
Hi! Nice work with this library. Too bad I didn't find this a week ago. Would've saved me from writing some obj-c myself. :)
Anyway, the current npm version doesn't work with react-native master / upcoming 0.7 release.
Short summary: the react-native packager has been rewritten, and require resolution doesn't work like it used to. The new packager fails to resolve
require('react-native-safari-view')
. Changing the main script as index.ios.js resolves this for now (I think it should work without any changes). Also,require('NativeModules')
is now deprecated;require('react-native').NativeModules
should be used instead.See facebook/react-native#1808 for some details.