-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(appconnect): Filter out app clip bundles since they don't have anything we can use #29704
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
Conversation
…ything we can use
Swatinem
left a comment
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.
nice catch!
| if safe.get_path(app_bundle, "attributes", "bundleType", default="APP") != "APP_CLIP" | ||
| ] | ||
|
|
||
| if len(bundles) == 0: |
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.
| if len(bundles) == 0: | |
| if len(app_bundles) == 0: |
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 folks really need to get used to the python notation of
| if len(bundles) == 0: | |
| if not app_bundles: |
In python using len() is not more explicit in this case.
flub
left a comment
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.
IIUC app clips are part of the app itself, so they get compiled. I've not found any information on whether they should ever have any dSYMs so am a bit uncomfortable for now assuming they don't. The example we have so far is exclusively for a single project, that's not much sample size.
Given we only query VALID builds currently (and not PENDING) I'm more tempted to instead accept a clip as 2nd buildBundle, see if it has a dSYM URL and download it as well if so.
I'd still keep the strategy of learning more about how this works by creating sentry errors for things we don't know. So I'd like to keep getting sentry errors for:
- multiple buildBundles where it is not one app and one app clip
- an APP_CLIP buildBundle which has a dSYM URL (even though we'd already download it)
| if safe.get_path(app_bundle, "attributes", "bundleType", default="APP") != "APP_CLIP" | ||
| ] | ||
|
|
||
| if len(bundles) == 0: |
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 folks really need to get used to the python notation of
| if len(bundles) == 0: | |
| if not app_bundles: |
In python using len() is not more explicit in this case.
|
@flub I think that's fair - I also think that we should probably talk to the mobile team about getting app clips set up for our test data so we can play with these a little. I'm still a little skeptical about app clips having their own dSYMs, or at least we should really investigate this because it's unclear to me how we would associate those dSYMs with their respective app clips, much less the app's dSYMs with the app clip if that's necessary. Given that app clips don't seem to exist in isolation separate from apps seeing as builds (not bundles) don't have an attribute to differentiate between app clips and apps, what does it mean for them to crash or throw errors? Are they essentially their own applications with their own dSYMs as far as we're concerned? Does this remotely match the way they're presented in Apple's schema? Regardless, I'm fine with keeping the things the way they are for now and just observing whatever it is that rolls in. I do think we should be setting up our own testing for this stuff if it isn't too much work, though. |
|
@relaxolotl I've created NATIVE-362 to track this |
|
So for now handling multiple dSYM URLs is still a pain. I've instead added an assertion to be notified if your suspicion that app clips do not have dSYMs proves to be wrong. I think that's reasonable. |
|
gotta patch up a typing thing from the lambda and i'll merge this friend |
We've started getting some info on apps that have multiple build bundles, and it looks like one way this can happen is when an app has app clips. Build bundles for app clips don't really contain any useful info (ie a majority of their attributes are empty) which makes sense; they're just a fancy link to the application and shouldn't contain any real logic. This filters the build bundles for app clips out.