-
Notifications
You must be signed in to change notification settings - Fork 904
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: ignore non-nativetargets during ios linking #123
Conversation
+1 really good job imho |
On second glance, are we sure there are not some cases where there is silent failing that needs to be handled (I think definitely yes and they need to be documented in this pr thread.) |
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.
Thank you for submitting this PR! It's great that you have provided a reproduction (cloned locally and tested).
Please help me understand under what circumstances such target can be invalid and how one could create it manually.
It would be also great to have a test case for that. It would require modifying fixtures tho.
const key = target.value; | ||
// If not a valid target, skip. | ||
if (!project.pbxNativeTargetSection()[key]) return undefined; | ||
const configurationListId = project.pbxNativeTargetSection()[key] |
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.
While we are doing this, let's make it: nativeTargetSection[key]
since we already have native target section obtained on line 17.
.map(target => { | ||
const key = target.value; | ||
// If not a valid target, skip. | ||
if (!project.pbxNativeTargetSection()[key]) return undefined; |
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.
Let's flip filter
and map
order, e.g:
targets.filter(target => nativeTargetSection[target.value] !== undefined).map(...)
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.
It would be also great to elaborate a bit more on the comment itself. For example, I would say: "Filter out targets that are ....". I am trying to understand what exactly is an invalid target (if it's in targets section and not in the native targets section - what does it mean).
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.
I run into the problem whenever I have an aggregate target alongside the default ones created by the cli. From the looks of it, we only need to modify native targets when linking, so anything not in the Native Target section should be ignored.
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.
Oh, now I remember! Due to limitations of Xcode library, I had to enumerate native targets by getting an array of project targets first. But yeah, I wrongly assumed that all targets will be in that section, which is not the case.
Makes use of the single variable nativeTargetSection and puts filter before map when filtering out non-native targets.
@grabbou |
fixes issue #57