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

feat(apple): Add support for iOS #334

Merged
merged 26 commits into from
Jul 11, 2023
Merged

feat(apple): Add support for iOS #334

merged 26 commits into from
Jul 11, 2023

Conversation

brustolin
Copy link
Contributor

Added the flow to add Sentry to an iOS project.

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against efbdd1c

@brustolin
Copy link
Contributor Author

This PR is big already, I will open for review and add support for cocoapod and fastlane in another one.

@brustolin brustolin marked this pull request as ready for review July 5, 2023 14:28
@smeubank
Copy link
Member

smeubank commented Jul 5, 2023

@HazAT FYI

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Looks good to me from a JS dev perspective :) Can't say much about the code snippets/SDK configuration specifics but I think the general wizard flow looks good. I had some comments/suggestions.

lib/Steps/Integrations/Apple.ts Outdated Show resolved Hide resolved
lib/Steps/Integrations/Apple.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/utils/clack-utils.ts Outdated Show resolved Hide resolved
@brustolin brustolin requested a review from Lms24 July 6, 2023 07:05
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks for adding my suggestions!

One more thing:

image

For folks who start the wizard without the -i arg, I think the grouping here isn't ideal. Wdyt about moving iOs to the top or below RN? I think we should sort the selection here roughly by platform (mobile, browser/JS + utilities like source maps).

At some point, depending on how many other wizards we add, we might want to somehow make these groups more apparent but for now I'd say moving iOS up should be good.

return;
}

const pbxproj = path.join(projectDir, xcodeProjFile, "project.pbxproj");
Copy link
Member

Choose a reason for hiding this comment

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

Just to sanity-check (for someone who has no idea about xcode/ios apps): Is this a valid path?

wizard/wizard-apple/test.xcodeproj/project.pbxproj

Background: I briefly tried the wizard locally but couldn't get beyond the pbxproj step:

image

Copy link
Contributor Author

@brustolin brustolin Jul 6, 2023

Choose a reason for hiding this comment

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

Yes. The ".xcodeproj" is actually a folder that keeps project configuration files.

I think we should not show the "Successfully..." message in this case.

I briefly tried the wizard locally but couldn't get beyond the pbxproj step:

You just tried to run it, or you really have an xcode project to test it? Because it should have worked if you have a project.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. The ".xcodeproj" is actually a folder that keeps project configuration files.

Okay, all good then! I guess I was just confused by the xcodeProjectFile variable then 😅

You just tried to run it

Yeah, but not very methodically and without an actual xcode project. Just wanted to get over a few steps so I created some empty files 😅

Copy link
Member

Choose a reason for hiding this comment

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

@brustolin do you know a repo which would be a example app to test this stuff with ourselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/apple/apple-wizard.ts Show resolved Hide resolved
@brustolin
Copy link
Contributor Author

Wdyt about moving iOs to the top or below RN?

Sound good. Can we try to find out which project exists to reorder the list?
Lets say there is a .xcodeproj directory, we know this may be an iOS or RN project, I guess we have other rules to indicate a RN project.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, @brustolin 💪 . I added some comments.

src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/apple/code-tools.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Show resolved Hide resolved
src/apple/code-tools.ts Show resolved Hide resolved
src/apple/templates.ts Outdated Show resolved Hide resolved
src/apple/templates.ts Outdated Show resolved Hide resolved
src/apple/templates.ts Outdated Show resolved Hide resolved
Comment on lines 23 to 24
options.attachScreenshot = true
options.attachViewHierarchy = true
Copy link
Member

Choose a reason for hiding this comment

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

h: I don't think we should enable that by default. Especially the screenshots could contain sensitive data, even in a development build.

Copy link
Member

Choose a reason for hiding this comment

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

we could also consider having them //commented out so it is easy enough for the user to decide themselves

Copy link
Member

@romtsn romtsn Jul 7, 2023

Choose a reason for hiding this comment

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

if I may, we could just add a comment to the snippet explaining that it can have sensitive data and turn it off in case you don't want it. Or we could add another step "Enable recommended features (could send PII to Sentry)"? and only after accepting it add the snippet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea to show this options in the wizard and explain whats happening, but in another PR.

Copy link
Member

@philipphofmann philipphofmann Jul 10, 2023

Choose a reason for hiding this comment

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

I think adding a comment won't be sufficient. With the wizard, it can happen that some people don't check the code that much. They just hit compile and run after executing the wizard. We could add them //commented out, or an extra step would also be OK.

Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
@Lms24
Copy link
Member

Lms24 commented Jul 7, 2023

Can we try to find out which project exists to reorder the list?

We sort of do this in the ChooseIntegration step for RN and Cordova:

public tryDetectingIntegration(): Integration | undefined {
if (_.has(projectPackage, 'dependencies.react-native')) {
return Integration.reactNative;
}
if (_.has(projectPackage, 'dependencies.cordova')) {
return Integration.cordova;
}
return;
}
private async _getIntegrationPromptSelection(): Promise<IntegrationPromptAnswer> {
// If we receive project type as an arg we skip asking
if (this._argv.integration) {
return { integration: this._argv.integration };
} else {
if (this._argv.quiet) {
throw new Error('You need to choose a integration');
}
const detectedDefaultSelection = this.tryDetectingIntegration();
return prompt([
{
choices: getIntegrationChoices(),
default: detectedDefaultSelection,
message: 'What integration do you want to set up?',
name: 'integration',
type: 'list',
},
]);
}
}

However, this just influences the default selection in the list, not the order. I'd argue this should be enough.
Fwiw we didn't add detection for the JS frameworks (Next, SvelteKit) yet but it should be fairly straight-forward.

@smeubank smeubank linked an issue Jul 7, 2023 that may be closed by this pull request
@smeubank
Copy link
Member

smeubank commented Jul 7, 2023

This PR is big already, I will open for review and add support for cocoapod and fastlane in another one.

@brustolin sounds good 🚀 if you could link to the parent issue for those PRs when you get there just let us know.

only thing for me, before we make it customer facing is to ensure we have some telemetry with no PII, so we can track that it is working well. We identified a lot of simple edge cases with -sorucemaps wizard this way and are able to ship a lot of little improvements

#343

src/apple/xcode-manager.ts Fixed Show fixed Hide fixed
src/apple/xcode-manager.ts Fixed Show fixed Hide fixed
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM from the Cocoa SDK perspective.

src/apple/templates.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
src/apple/apple-wizard.ts Outdated Show resolved Hide resolved
brustolin and others added 2 commits July 10, 2023 16:53
@brustolin brustolin changed the title feat: Add support for iOS feat(apple): Add support for iOS Jul 11, 2023
@brustolin brustolin merged commit 3734b20 into master Jul 11, 2023
7 checks passed
@brustolin brustolin deleted the feat/apple-support branch July 11, 2023 08:24
@kahest kahest mentioned this pull request Jul 11, 2023
Lms24 added a commit that referenced this pull request Jul 14, 2023
Seems like a check in #334 unintentionally disabled telemetry collection by default. I missed this when reviewing the PR. As a consequence of this and merging in #360, we didn't collect any telemetry data in the last 3.7.0 release. This patch removes the unnecessary check and restores telemetry collection.

Furthermore, this patch also adjusts the welcome message to no longer show the telemetry collection note in the welcome message if it is disabled.
@brustolin brustolin mentioned this pull request Jul 18, 2023
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.

iOS cli-wizard
5 participants