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: add fallback behavior when running a default simulator on iOS #758

Merged
merged 7 commits into from Oct 5, 2019

Conversation

@Naturalclar
Copy link
Contributor

commented Sep 29, 2019

Summary:

This is an attempt to solve problem on #739, based on idea provided at #739 (comment)

This changes the behavior of runIOS when no simulator argument is provided.

When no simulator argument is provided, the cli will look for simulators in the following order

  • iPhone 11
  • iPhone X
  • iPhone 8

Test Plan:

yarn
yarn test
yarn lint

@Naturalclar Naturalclar changed the title Fix/issue739 feature: try for different iOS simulators when no simulator argument is provided Sep 29, 2019
Copy link
Member

left a comment

Thanks for tackling this! Looks good, left some feedback

@@ -492,7 +507,7 @@ export default {
description:
'Explicitly set simulator to use. Optionally include iOS version between' +
'parenthesis at the end to match an exact version: "iPhone 6 (10.0)"',
default: 'iPhone X',
default: '',

This comment has been minimized.

Copy link
@thymikee

thymikee Sep 29, 2019

Member

I think we can default to undefined (or just don't provide the default) – can we change that and adjust docs as well?

: 'iPhone 8';
}

const selectedSimulator = findMatchingSimulator(simulators, simulator);

This comment has been minimized.

Copy link
@thymikee

thymikee Sep 29, 2019

Member

this is calling findMatchingSimulator twice for iPhone 11 and iPhone X. Can we move this call to line 143 (iPhone 8) and get rid of selectedSimulator variable?

@Naturalclar

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

Thanks for the feedback! I've made some modification to reduce the number of times findMatchingSimulator run

@Naturalclar Naturalclar requested a review from thymikee Sep 30, 2019
Copy link
Contributor

left a comment

Thanks for sending this over! I think this is a really good idea. Left some feedback as well, on top of what @thymikee already wrote.

* - iPhone X
* - iPhone 8
*/
if (!simulator) {

This comment has been minimized.

Copy link
@grabbou

grabbou Oct 1, 2019

Contributor

I wonder if this could be replaced with a simple while loop or a for loop and break statement. Thoughts?

This comment has been minimized.

Copy link
@Naturalclar

Naturalclar Oct 2, 2019

Author Contributor

thanks for the suggestion!
I modified it to use array.some so that loop would break when simulator is found.

This comment has been minimized.

Copy link
@grabbou

grabbou Oct 3, 2019

Contributor

That is a good improvement, but still, I find doing side effects inside some a bit of a code smell.

How about:

for (const defaultSimulator of defaultSimulators) {
  selectedSimulator = findMatchingSimulator(simulators, defaultSimulator);
  if (selectedSimulator) {
     break;
  }
}

or

const selectedSimulator = args.simulator 
   ? findMatchingSimulator(simulators, args.simulator)
   : defaultSimulators.reduce(
       (selectedSimulator, simulator) => selectedSimulator || findMatchingSimulator(simulators, simulator),
       null
     );

The below (in my opinion) would be the most concise:

const selectedSimulator = defaultSimulators.reduce(
   (selectedSimulator, simulator) => selectedSimulator || findMatchingSimulator(simulators, simulator),
   findMatchingSimulator(simulators, args.simulator)
);

but it would require we bring: args.simulator default value to be iPhone 11 and rename defaultSimulators to be fallbackSimulators (in other words, try these ones in case user provided/default value does not exist on the machine).

Leaving that up to further discussion.

This comment has been minimized.

Copy link
@Naturalclar

Naturalclar Oct 4, 2019

Author Contributor

Thank you for the feedback and suggestions!
I find doing side effects inside some feels iffy as well, but was not able to come up with a solution at the moment.

I like the last suggestion of providing fallback when args.simulator is not available. It let me remove multiple if s.

I've modified the default simulator to be iPhone 11 and modified the document to match the code.

@Naturalclar Naturalclar requested a review from grabbou Oct 2, 2019
}
} else {
selectedSimulator = findMatchingSimulator(simulators, simulator);
if (!selectedSimulator) {

This comment has been minimized.

Copy link
@grabbou

grabbou Oct 3, 2019

Contributor

I believe we can move this and another if statement at line 152 outside of this block. If we would place it below line 163, we would be able to keep only one if statement, instead of the two.

* - iPhone X
* - iPhone 8
*/
if (!simulator) {

This comment has been minimized.

Copy link
@grabbou

grabbou Oct 3, 2019

Contributor

That is a good improvement, but still, I find doing side effects inside some a bit of a code smell.

How about:

for (const defaultSimulator of defaultSimulators) {
  selectedSimulator = findMatchingSimulator(simulators, defaultSimulator);
  if (selectedSimulator) {
     break;
  }
}

or

const selectedSimulator = args.simulator 
   ? findMatchingSimulator(simulators, args.simulator)
   : defaultSimulators.reduce(
       (selectedSimulator, simulator) => selectedSimulator || findMatchingSimulator(simulators, simulator),
       null
     );

The below (in my opinion) would be the most concise:

const selectedSimulator = defaultSimulators.reduce(
   (selectedSimulator, simulator) => selectedSimulator || findMatchingSimulator(simulators, simulator),
   findMatchingSimulator(simulators, args.simulator)
);

but it would require we bring: args.simulator default value to be iPhone 11 and rename defaultSimulators to be fallbackSimulators (in other words, try these ones in case user provided/default value does not exist on the machine).

Leaving that up to further discussion.

@Naturalclar Naturalclar requested a review from grabbou Oct 4, 2019
@codler
codler approved these changes Oct 5, 2019
Copy link
Member

left a comment

Let's ship it

const fallbackSimulators = ['iPhone X', 'iPhone 8'];
const selectedSimulator = fallbackSimulators.reduce((simulator, fallback) => {
return simulator || findMatchingSimulator(simulators, fallback);
}, findMatchingSimulator(simulators, args.simulator));

This comment has been minimized.

Copy link
@thymikee

thymikee Oct 5, 2019

Member

This loop always run through all simulators from fallbackSimulators, but since the data set is small and we avoid running findMatchingSimulator if necessary, I'm fine with that.

@thymikee thymikee dismissed grabbou’s stale review Oct 5, 2019

feedback addressed

@thymikee thymikee changed the title feature: try for different iOS simulators when no simulator argument is provided feat: add fallback behavior when running a default simulator on iOS Oct 5, 2019
@thymikee thymikee merged commit 77af493 into react-native-community:master Oct 5, 2019
8 checks passed
8 checks passed
ci/circleci: cocoa-pods Your tests passed on CircleCI!
Details
ci/circleci: e2e-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lts-tests Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: typecheck Your tests passed on CircleCI!
Details
ci/circleci: unit-tests Your tests passed on CircleCI!
Details
react-native-community.cli Build #20191004.2 succeeded
Details
sonicdoe added a commit to figureaps/cli that referenced this pull request Oct 9, 2019
sonicdoe added a commit to figureaps/cli that referenced this pull request Oct 9, 2019
@sonicdoe sonicdoe referenced this pull request Oct 9, 2019
thymikee added a commit that referenced this pull request Oct 11, 2019
Co-authored-by: Jakob Krigovsky <jakob@krigovsky.com>
sonicdoe added a commit to figureaps/cli that referenced this pull request Oct 11, 2019
@sonicdoe sonicdoe referenced this pull request Oct 11, 2019
thymikee added a commit that referenced this pull request Oct 11, 2019
… (#782)

Co-authored-by: Jakob Krigovsky <jakob@krigovsky.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.