-
Notifications
You must be signed in to change notification settings - Fork 934
feat(ios): cache last used device #2162
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
Conversation
| devices: Device[], | ||
| lastUsedIOSDeviceId?: string, | ||
| ): Promise<Device | undefined> { | ||
| const sortedDevices = devices; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sortedDevices still references devices and is then mutated. You'd need to copy the devices array to avoid mutating it with unshift on line 49 and splice on 48
| devices: Device[], | ||
| lastUsedIOSDeviceId?: string, | ||
| ): Promise<Device | undefined> { | ||
| const sortedDevices = [...devices]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these devices sorted? I don't think they are... This line seems a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sorted here, but I needed to create a value to do not mutate params.
cipolleschi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, but overall looks good to me.
8e481d2 to
c1fe6ae
Compare
Summary:
Developers often have a lot of simulators installed on their machines, and if they are running
--list-devicesor--interactivethey need to go through all of them, but by adding simple caching we'll move last used device to the top of the list.Uploading CleanShot 2023-11-07 at 17.06.00.mp4…
Test Plan:
Checklist