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

Distinct platform dependencies check #262

Merged
merged 7 commits into from
May 21, 2024

Conversation

jakub-gonet
Copy link
Member

@jakub-gonet jakub-gonet commented May 15, 2024

This PR allows using iOS and Android independently.

If Android/iOS tools are not installed, we block managing them, as well as disabling buttons for creating new devices in empty state.
We also don't show notification about missing dependencies on the other platform.

I don't particularly like isSomethingError API, the alternative is to provide errors object with scoped domains or access .error field on dependencies directly.

Screenshots/videos

android.mov
ios.mov

Test plan

When using studio for the first time:

  • and Xcode or Android isn't setup – we show an error after clicking "Add Android/iPhone".
  • when using "Create new device" broken options aren't shown in the menu.
    After creating new device
  • iOS/Android setup doesn't show errors if not used.

Switching between devices still works (we don't handle when environment change when IDE is open).

Known error: we show "Check diagnostics" message in creation modal but it's not accessible in the empty view.

Fixes #173

@jakub-gonet jakub-gonet requested a review from kmagiera May 15, 2024 09:53
Copy link

vercel bot commented May 15, 2024

@jakub-gonet is attempting to deploy a commit to the software-mansion Team on Vercel.

To accomplish this, @jakub-gonet needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@jakub-gonet jakub-gonet requested review from kacperkapusciak and kmagiera and removed request for kmagiera and kacperkapusciak May 15, 2024 09:54
@jakub-gonet
Copy link
Member Author

Seems like I can assign only one reviewer. I'd you both to look at it, if possible.

@jakub-gonet jakub-gonet force-pushed the jgonet/platform-dependencies-check branch from 897c453 to 8ce7292 Compare May 20, 2024 20:21
async function createDevice() {
if (!selectedSystemName) {
return;
}

setLoading(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

I left them as they were but early returns in this function seem like an error – we never call setLoading(false) then. Same for empty devices list view.

const firstIosDeviceName = iOSSupportedDevices[0].name;
const firstAndroidDeviceName = AndroidSupportedDevices[0].name;

function getMax<T>(array: T[], predicate: (element: T, currentMax: T) => boolean): T | undefined {
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe too abstract?

<span className="codicon codicon-add" />
Create new device
</Button>
<>
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Remove empty tags.

@@ -21,7 +21,7 @@ class VSCodeAPIWrapper {
* @remarks When running webview code inside a web browser, postMessage will instead
* log the given message to the console.
*
* @param message Abitrary data (must be JSON serializable) to send to the extension context.
* @param {{command: string} & Record<string, unknown>} message - Arbitrary data (must be JSON serializable) to send to the extension context.
Copy link
Member Author

Choose a reason for hiding this comment

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

To we get any typing.

import { Logger } from "../Logger";
import { extensionContext } from "../utilities/extensionContext";
import { WorkspaceConfigController } from "./WorkspaceConfigController";
import { getTelemetryReporter } from "../utilities/telemetry";

type CallArgs = {
Copy link
Member Author

Choose a reason for hiding this comment

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

method and args could be typed more precisely but it's a good start I think.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

There are way too many changes than I expected from this PR. Nothing really stands out here as wrong but I might have missed things, so I hope you've tested the changes. Please add test plan to this PR before merging.

@jakub-gonet jakub-gonet merged commit 609f2ac into main May 21, 2024
1 of 2 checks passed
@jakub-gonet jakub-gonet deleted the jgonet/platform-dependencies-check branch May 21, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to run without Android Emulator?
2 participants