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

fix all Framework imports that would break cocoapods installation #319

Merged
merged 4 commits into from
Jun 4, 2018

Conversation

DulMephistos
Copy link

This PR will fix cocoapods integration for the libraries appcenter, appcenter-crashes and appcenter-push. They're currently failing due to wrong use of @import statements inside its headers and implementation files. These libraries should build without the need to import AppCenter directly, just by knowing theirs .h files.

The issue can be demonstrated if you try to integrate using purely cocoapods and not by adding the xcode project files inside your project. If done so you should get an error like this one:

▸ Compiling AppCenterReactNative.m

AppCenterReactNativeShared/AppCenterReactNativeShared/AppCenterReactNativeShared.h:3:9: module 'AppCenter' not found

This change removes that import from the header file and replaces it with a Forward Declaration of the class or protocol if needed. Then inside the implementation file the needed header is imported using #import <Framework/Header.h> format instead of the module format, that should work for all cases.

@msftclas
Copy link

msftclas commented May 24, 2018

CLA assistant check
All CLA requirements met.

@@ -1,7 +1,5 @@
#import <Foundation/Foundation.h>

@import AppCenter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need some header for MSWrapperSdk type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also in AppCenterReactNativeShared.m file we need header for MSAppCenter type

Copy link
Author

Choose a reason for hiding this comment

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

You're right.. I wasn't able to properly build the framework before, not sure why but it should be good to go now. Built and checked.
I've use forward declaration on the header and imported the right header in the .m file, still not requiring a @import which has been the issue for our integration flow.

@@ -11,6 +9,7 @@
#endif

@class AppCenterReactNativePush;
@protocol MSPushDelegate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing empty line before protocol

Copy link
Author

Choose a reason for hiding this comment

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

@DulMephistos
Copy link
Author

DulMephistos commented May 29, 2018

let me know how does it look now guys. Thank you!

Copy link
Contributor

@bmourat bmourat left a comment

Choose a reason for hiding this comment

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

The build fails for the TestApp, as for the newly created app. Here is the error message:
...../TestApp/node_modules/appcenter/ios/AppCenterReactNative/AppCenterReactNative.m:20:9: 'AppCenterReactNativeShared/AppCenterReactNativeShared.h' file not found.
Adding folder to Headers search path fixes the issue.
I think this was not the case because we were using import module syntax, but this is not 100%.

@DulMephistos
Copy link
Author

That's odd because here it's all normal. I've run the script inside the TestApp called update-link-to-appcenter.sh, then opened Xcode and it built everything without any issues on my machine. Check this screenshot that specifically shows the compile log of this file.
screen shot 2018-05-30 at 4 35 20 pm

The only two things that I noticed were:

  • The header inside the /AppCenterReactNativeShared.framework was not updated (just committed that)
  • And that Xcode produces a lot of changes after linking with that script, but I believe those are just a bunch of IDs that change inside .xcodeproj file on linking and will change every time.

Please let me know if you have more documentation on how to run all the tests you guys run on a PR. I might have missed something in the process. I didn't find docs for contributing.

@bmourat
Copy link
Contributor

bmourat commented May 31, 2018

That's strange now everything compiles, maybe that is because I was performing all those operations manually and missed something.

@bmourat bmourat mentioned this pull request May 31, 2018
@DulMephistos
Copy link
Author

@bmourat hey.. any idea of when this will be merged into develop or master? We're currently blocked on this to make AppCenter integration work on iOS. Thanks!

@guperrot
Copy link
Member

guperrot commented Jun 1, 2018

Hi, we release our SDKs the third week of the month. If you need packages before you can run scripts/npm-pack.sh on your branch and use those locally or in a private or non official repo. As for merging we will do it soon but release will come later after the merge.

@DulMephistos
Copy link
Author

Great! thanks for the quick response @guperrot

@dhei dhei merged commit 12efbe9 into microsoft:develop Jun 4, 2018
@dhei
Copy link
Member

dhei commented Jun 4, 2018

Hey @fcerdeiral, thanks for the contribution, your PR is merged and will be shipped in our June SDK release.

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.

5 participants