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

Fixed issue 192 #193

Closed
wants to merge 2 commits into from
Closed

Conversation

marchy
Copy link

@marchy marchy commented Oct 31, 2015

Fixed issue 192 (#192), allowing projects that include frameworks which themselves include Bolts to work without running into “Include of non-modular header inside framework module” compilation errors.

The problem here was that there were dependencies included directly in the .h file instead of in the .m files (see here: http://stackoverflow.com/questions/28552500/xcode6-receiving-error-include-of-non-modular-header-inside-framework-module).

Instead these were moved to use the proper @Class and @protocol definitions in the headers and only including the actual file in the .m file. This removes the circular dependency issues so that the Objective-C compiler can properly make everything work in projects that include sub-dependencies (ie: include a framework which it itself includes the Bolts library)

…frameworks which themselves include Bolts to work without running into “Include of non-modular header inside framework module” compilation errors.
@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@nlutsenko
Copy link
Member

Let's have a broader discussion in #192.
The inclusion of dependencies in the framework header files is supported use case and used everywhere, say in Foundation/UIKit frameworks.

@nlutsenko nlutsenko self-assigned this Nov 2, 2015
@marchy marchy force-pushed the fix-for-issue-192 branch from 7a883f3 to 5337e71 Compare November 2, 2015 17:25
@marchy
Copy link
Author

marchy commented Nov 2, 2015

The fix above solves the issues. This was only problematic in 3 files so it's a fairly small change. Summary of changes:

  • Changed from use of imports to class forwarding
  • Moved the two protocols that were defined in the headers into their own respective files (BFAppLinkNavigationType.h and BFAppLinkReturnToRefererViewDelegate.h)

This does solve the issue and is either way a best-practice in general, improving compilation speed if nothing else. Please have a look and accept the pull request if this works. Clearly the issue is happening for some people and it's much better to solve these issues rather than depend on workarounds such as CLANG_ALLOW_NON_MODULAR_INCLUDES_IN_FRAMEWORK_MODULES = YES to get around them (http://stackoverflow.com/questions/24103169/swift-compiler-error-non-modular-header-inside-framework-module)

@@ -8,7 +8,6 @@
*
*/

#import <Bolts/BoltsVersion.h>
Copy link
Member

Choose a reason for hiding this comment

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

This is still important to have.

@nlutsenko
Copy link
Member

Few nits here and there... I am sorry, but this still seems not necessary per discussion outlined in #192.

Class forwarding is a good approach, I absolutely agree though, but we are not using it in a lot of places, so I am somewhat sceptical about actual compilation impact.

@nlutsenko
Copy link
Member

Hey @marchy, I am going to close out this pull request as it looks abandoned.
Feel free to comment or rebase & reopen it.

@nlutsenko nlutsenko closed this Mar 29, 2016
@facebook-github-bot
Copy link
Contributor

@marchy updated the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants