-
Notifications
You must be signed in to change notification settings - Fork 46
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
Update Windows project to fix compilation and add auto-linking #55
Update Windows project to fix compilation and add auto-linking #55
Conversation
@@ -162,18 +161,25 @@ | |||
</ProjectReference> | |||
</ItemGroup> | |||
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" /> | |||
<ImportGroup Label="ReactNativeWindowsTargets"> |
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.
Adding the import of the CppLib.targets is correct, but you also need to add the import for the CppLib.props earlier in the file, and remove the project references for Microsoft.ReactNative and Microsoft.ReactNative.Cxx.
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.
You can see similar changes I made for react-native-video's project here: https://github.com/react-native-video/react-native-video/pull/2206/files#diff-21a98c9b66b676799e297b1bf6028f32087df1baf8ebbc109181f75e8348a4e6
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.
Thanks for the feedback, I'll update the PR with your suggestions.
@Naturalclar You seem to have merged the last few PRs, could you take a look at this one? |
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.
LGTM, ping @jonthysell in case he has any concerns.
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.
@asklar thanks for the review!
@tritao the change has been published in v1.2.4 🎉 thanks for the change! |
Should fix #50