cli icon indicating copy to clipboard operation
cli copied to clipboard

fix: update logic to check for available ios devices for `log-ios` to work

Open esthor opened this issue 2 years ago • 5 comments

Summary:

Hey, just ran into this while working on a new feature.

When pulling the devices via xcrun simctl list devices --json now, I don't actually get the availability key, I just get the isAvailable key. They are both already listed as optional keys in the Device type in the repo. My guess is that the availability key was on an older version of simctl.

Anyway, if I run the log-ios as-is, without this change, it always fails to find an available iOS device because device.availability simply never exists. Changing it to also check for device.isAvailable works.

Test Plan:

environment: macOS 12.5 (latest stable) xcrun version 61 / xcode 13.4.1 (latest stable)

  1. Have an iOS simulator open and running
  2. Run the log-ios command as is in the current main cli branch. (Result: FAILS with error: No active iOS device found)
  3. Run the log-ios command with this PR's code. (Result: Successfully finds the active simulator and starts a log session)

esthor avatar Aug 12 '22 16:08 esthor

Two things that we might want to follow-up on:

  1. update tests (the findMatchingSimulator.test.ts still just passes device.availability mocks, so it should probably have some mocks with device.isAvailable)
  2. make a shared variable for availability so we don't repeat ourselves in the future and can update the logic in just one place?

(please lmk if these are needed before merging or anything else)

esthor avatar Aug 12 '22 16:08 esthor

It looks like we have some overlapping implementations, and this is properly resolved in run-ios, but it haven't propagated to log-ios: https://github.com/react-native-community/cli/blob/b3812eac55dd979bb82b078f347fd45f69dc4392/packages/cli-platform-ios/src/commands/runIOS/findMatchingSimulator.ts#L70-L73 Would you be so kind and refactor this so more code is being shared, to avoid such discrepancies in the future?

thymikee avatar Aug 17 '22 19:08 thymikee

Would you be so kind and refactor this so more code is being shared, to avoid such discrepancies in the future?

Yes, 💯 . I wanted to propose this, but didn't want to risk rocking the boat too much. Happy to hop on it!

esthor avatar Aug 22 '22 19:08 esthor

Would you be so kind and refactor this so more code is being shared, to avoid such discrepancies in the future?

Yes, 💯 . I wanted to propose this, but didn't want to risk rocking the boat too much. Happy to hop on it!

Go for it! 💪

adamTrz avatar Aug 25 '22 11:08 adamTrz

Would you be so kind and refactor this so more code is being shared, to avoid such discrepancies in the future?

Yes, 💯 . I wanted to propose this, but didn't want to risk rocking the boat too much. Happy to hop on it!

Go for it! 💪

Will do!

esthor avatar Sep 12 '22 17:09 esthor

@esthor is there anything we can help you with to push this through?

thymikee avatar Oct 31 '22 10:10 thymikee

I'm adding it to my TODO in a week 🙏

esthor avatar Nov 13 '22 22:11 esthor

There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.

github-actions[bot] avatar Feb 12 '23 03:02 github-actions[bot]

My bad, I thought this was handled in other changes, but it's not. I'll find some time to try and wrap up this PR.

esthor avatar Feb 14 '23 17:02 esthor

Hey @esthor, recently @adamTrz fixed error with getting devices and simulators in CLI https://github.com/react-native-community/cli/pull/1823 and basically there was a change from xcrun xctrace list devices to xcrun xcdevice list --json, and this new command outputs the beautiful JSON with all required informations together with avaliable. We're thinking about moving to this new command also in log-ios . Because it's safer (we're not doing some crazy regex, and after that tons of difficult logic instead we're just getting information from JSON).

szymonrybczak avatar Feb 28 '23 18:02 szymonrybczak