Skip to content

refactor(cli-platform-apple): move installing and launching app logic to separate function - #2234

Merged
thymikee merged 2 commits into
mainfrom
chore/refactor-install-app
Jan 3, 2024
Merged

refactor(cli-platform-apple): move installing and launching app logic to separate function#2234
thymikee merged 2 commits into
mainfrom
chore/refactor-install-app

Conversation

@szymonrybczak

@szymonrybczakszymonrybczak commented Dec 22, 2023

Copy link
Copy Markdown
Collaborator

Summary:

Moves installing and launching app logic to separate function, so that adding additional logic for another platforms will be fairly easy.

Test Plan:

  1. Clone the repository and do all the required steps from the Contributing guide
  2. Run this command:
node /path/to/react-native-cli/packages/cli/build/bin.js run-ios

Works same as before.

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

@github-actionsgithub-actionsBot added the infra Internal work not facing public APIs label Dec 22, 2023
@thymikee
thymikeeforce-pushed the chore/refactor-install-app branch from 0a265a5 to b4043bdCompareDecember 27, 2023 10:51
thymikee
thymikee reviewed Dec 27, 2023
View reviewed changes
Comment threadpackages/cli-platform-apple/src/commands/runCommand/getBuildPath.ts Outdated
buildOutput: string,
scheme: string,
target: string | undefined,
buildSettings: any,

@thymikeethymikeeDec 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we refine that type?

@cipolleschicipolleschiJan 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be of type BuildSettings from here

thymikee
thymikee reviewed Dec 27, 2023
View reviewed changes
Comment threadpackages/cli-platform-apple/src/commands/runCommand/getBuildPath.ts
thymikee
thymikee reviewed Dec 27, 2023
View reviewed changes
Comment threadpackages/cli-platform-apple/src/commands/runCommand/getBuildSettings.ts
thymikee
thymikee reviewed Dec 27, 2023
View reviewed changes
Comment threadpackages/cli-platform-apple/src/commands/runCommand/getBuildSettings.ts
thymikee
thymikee reviewed Dec 27, 2023
View reviewed changes
Comment threadpackages/cli-platform-apple/src/commands/runCommand/installApp.ts
}

type Options = {
buildOutput: any;

@thymikeethymikeeDec 27, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's add some basic type. According to data flow this can be undefined and I'm not sure if it's handled properly

thymikee
thymikee reviewed Dec 27, 2023
View reviewed changes
Comment threadpackages/cli-platform-apple/src/commands/runCommand/installApp.ts Outdated
@szymonrybczak
szymonrybczakforce-pushed the chore/refactor-install-app branch 2 times, most recently from c7efeca to 140c015CompareJanuary 2, 2024 16:54
szymonrybczak
szymonrybczak commented Jan 2, 2024
View reviewed changes
Comment threadpackages/cli-platform-apple/src/commands/runCommand/installApp.ts
Comment on lines +54 to +60
if (!buildSettings) {
throw new CLIError('Failed to get build settings for your project');
}

@szymonrybczakszymonrybczakJan 2, 2024•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's also not the most informative error ever, soon in a separate PR I'll fix all errors in this place :)

cipolleschi
cipolleschi approved these changes Jan 3, 2024
View reviewed changes
Comment threadpackages/cli-platform-apple/src/commands/runCommand/getBuildPath.ts Outdated
buildOutput: string,
scheme: string,
target: string | undefined,
buildSettings: any,

@cipolleschicipolleschiJan 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be of type BuildSettings from here

@szymonrybczak
szymonrybczakforce-pushed the chore/refactor-install-app branch from 140c015 to 99331ccCompareJanuary 3, 2024 12:56
@szymonrybczak
szymonrybczakforce-pushed the chore/refactor-install-app branch from 99331cc to 31e7718CompareJanuary 3, 2024 13:20
@szymonrybczakszymonrybczak mentioned this pull request Jan 3, 2024
Merged
2 tasks
@thymikee
thymikee merged commit 296e640 into mainJan 3, 2024
@thymikee
thymikee deleted the chore/refactor-install-app branch January 3, 2024 15:06
@szymonrybczakszymonrybczak mentioned this pull request Feb 28, 2024
Merged
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@cipolleschicipolleschicipolleschi approved these changes

Assignees

No one assigned

Labels

infraInternal work not facing public APIs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@szymonrybczak@thymikee@cipolleschi