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

0.7 support #1

Merged
merged 3 commits into from
Jul 21, 2015
Merged

0.7 support #1

merged 3 commits into from
Jul 21, 2015

Conversation

mnylen
Copy link
Contributor

@mnylen mnylen commented Jul 4, 2015

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.

Mikko Nylén added 3 commits July 4, 2015 23:24
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')
@naoufal
Copy link
Owner

naoufal commented Jul 5, 2015

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": {
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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.

@mnylen
Copy link
Contributor Author

mnylen commented Jul 5, 2015

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.

@naoufal
Copy link
Owner

naoufal commented Jul 5, 2015

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.

@cheeaun
Copy link

cheeaun commented Jul 19, 2015

Just FYI, 0.71 was released 9 days ago so wondering if this can be merged 😄

@naoufal
Copy link
Owner

naoufal commented Jul 21, 2015

@cheeaun @mnylen I was hoping this PR wouldn't be needed but looks like the packager issue hasn't been resolved yet.

I'm going to merge and publish to unblock you guys.

@mnylen thanks again for your contribution. 👍

naoufal added a commit that referenced this pull request Jul 21, 2015
@naoufal naoufal merged commit 004c102 into naoufal:master Jul 21, 2015
@naoufal
Copy link
Owner

naoufal commented Jul 21, 2015

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

3 participants