feat: add fallback behavior when running a default simulator on iOS

[Naturalclar]

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

[thymikee]

Thanks for tackling this! Looks good, left some feedback

[thymikee]

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

[thymikee]

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]

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

[grabbou]

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.

[grabbou]

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

[Naturalclar]

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

[grabbou]

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]

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.

[grabbou]

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.

[grabbou]

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.

[thymikee]

Let's ship it

[thymikee]

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.

feedback addressed