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

[RN 0.40] Update iOS header imports and JS SyntheticEvent import for RN 0.40 #923

Merged
merged 2 commits into from Jan 6, 2017

Conversation

ide
Copy link
Contributor

@ide ide commented Dec 31, 2016

React Native 0.40 (coming soon) changes the way its iOS headers should be imported. The headers are now under a directory called "React" that is in the compiler's include path, hence the new import style.

On the JS side, RN 0.40 now peer-depends on React 15.4.x which moved SyntheticEvent from react/lib to deep inside react-native, so update that import as well.

Test Plan: Follow the setup instructions (https://github.com/airbnb/react-native-maps/blob/master/docs/examples-setup.md) and run the iOS explorer app (notably verify that it compiles). Here it is running, nothing special, it just works as expected:

screenshot 2016-12-30 21 33 42

"react-native": "^0.35.0",
"react-native-maps": "../"
"react": "~15.4.1",
"react-native": "^0.40.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

0.40 hasn't been published yet so this doesn't actually work yet but it will soon (I tested with github:facebook/react-native#0.40-stable)

@ide ide force-pushed the react-native-40 branch 3 times, most recently from f47a8f6 to ce1bf8b Compare January 3, 2017 23:14
@jpgarcia
Copy link

jpgarcia commented Jan 5, 2017

Any plans to merge this one soon?

"react-native-maps": "../"
"react": "~15.4.1",
"react-native": "^0.40.0",
"react-native-maps": "path:../"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using a version of npm that supports this syntax? I get this error:

npm ERR! Unsupported URL Type: path:../

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched this to "file:", verified this works with npm 3 and 4

@gilbox
Copy link
Contributor

gilbox commented Jan 5, 2017

I'd like to @jpgarcia ... were you able to get the example app to compile? I'm having two problems, see my comments above.

@jpgarcia
Copy link

jpgarcia commented Jan 5, 2017

didn't try it yet, maybe @ide can give us some insights

@ide
Copy link
Contributor Author

ide commented Jan 5, 2017

There might be a problem with the upstream podspec, I'm getting compiler errors inside of RN's source code.

…RN 0.40

React Native 0.40 (coming soon) changes the way its iOS headers should be imported. The headers are now under a directory called "React" that is in the compiler's include path, hence the new import style. On the JS side, RN 0.40 now peer-depends on React 15.4.x which moved SyntheticEvent from `react/lib` to deep inside `react-native`, so update that import as well.

Test Plan: Follow the setup instructions (https://github.com/airbnb/react-native-maps/blob/master/docs/examples-setup.md) and run the explorer app (notably verify that it compiles).
@ide
Copy link
Contributor Author

ide commented Jan 5, 2017

The problem is that the React podspec doesn't support frameworks. I'll push a working frameworkless Podfile in a separate commit so you can try it out.

Tested it with Google Maps support.
@ide
Copy link
Contributor Author

ide commented Jan 5, 2017

I pushed a new Podfile and verified the example project works with CocoaPods 1.1.1 and Xcode 8.2. When you run pod install you will get screenfuls of warnings -- ignore these, they are due to the React.podspec and you get them in any project.

@gilbox
Copy link
Contributor

gilbox commented Jan 5, 2017

Now I can compile but get this:

image

@ide
Copy link
Contributor Author

ide commented Jan 5, 2017

Are you running in a clean checkout? I'd try git clean -dfx, also run watchman watch-del-all to remove more possible cruft.

@gilbox
Copy link
Contributor

gilbox commented Jan 5, 2017

Same thing :-/ ... @spikebrehm wanna give it a go?

@ide
Copy link
Contributor Author

ide commented Jan 5, 2017

Maybe you need to clear the RN packager cache too with npm start -- --reset-cache. I went through the build steps again from scratch to verify the explorer app runs for me and it's working for me. This is what I did:

git clean -dfx
cd example
yarn  # npm install should be fine too though
pod install  # see a bunch of yellow warnings, but the workspace gets set up fine
watchman watch-del-all
npm start -- --reset-cache  # just to be sure the packager cache isn't bad
open AirMapsExplorer.xcworkspace
# run iOS app on iPhone 7 simulator

@spikebrehm
Copy link

I tested locally and it works great 👍

@spikebrehm spikebrehm merged commit fcc2995 into react-native-maps:master Jan 6, 2017
@martsie
Copy link
Contributor

martsie commented Jan 6, 2017

+1 Tested locally and it's now working.

Currently, the latest release version of react-native-maps doesn't work with the latest version of react-native despite the project page saying "Due to the rapid changes being made in the React Native ecosystem, we are not officially going to support this module on anything but the latest version of React Native."

Suggesting approving this PR or amending the project page for the time being if that's quicker.

@ide
Copy link
Contributor Author

ide commented Jan 6, 2017

@codesidekick This PR has been merged, npm installing from airbnb/react-native-maps should work now.

@spikebrehm
Copy link

Released as v0.13.0

@merapi
Copy link

merapi commented Jan 12, 2017

Related issue (in publishing version):
#957

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

6 participants