-
Notifications
You must be signed in to change notification settings - Fork 53
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
Implement GUI for selecting eas build configuration #932
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/vscode-extension/src/webview/views/LaunchConfigurationView.tsx
Outdated
Show resolved
Hide resolved
packages/vscode-extension/src/webview/views/LaunchConfigurationView.tsx
Outdated
Show resolved
Hide resolved
packages/vscode-extension/src/webview/views/LaunchConfigurationView.tsx
Outdated
Show resolved
Hide resolved
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.
I left some inline comments
async getAvailableEasProfiles(): Promise<string[]> { | ||
const appRootFolder = getAppRootFolder(); | ||
const easConfig = await readEasConfig(appRootFolder); | ||
const easBuildConfig = easConfig?.build ?? {}; |
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.
"build": {
"development": {
"developmentClient": true,
"distribution": "internal",
"ios": {
"simulator": true
}
},
"preview": {
"distribution": "internal"
},
"production": {
"autoIncrement": true
}
this is an example eas configuration (from eas.json
) we should only show profiles that have distribution
field set to "internal". And simulator
set to true. It is not necessary true, that profiles with those flag will generate dev builds runnable on simulators, but expo asumes so and you would have to do a lot of manula configuration to "cheat" that.
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.
I've thought about it. I'm not sure we should hide the "invalid" configs.
I feel like ideally we'd show all configs, but disable selecting the "invalid" ones with some information to the user why they are not selectable, but I'm not sure how much work it is to add that functionality to our components.
I'd leave this out of this PR.
return ( | ||
<div className="launch-configuration-container"> | ||
<div className="setting-description">{prettyPlatformName(platform)} Build Profile:</div> | ||
<Select |
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.
We should allow additional manual input of a value that is not on the list for users with exotic configurations, the button that does it might be hidden as a last psoittion in the select list for example.
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.
Possibly.
I feel like if you have an "exotic configuration", you can be asked to edit the launch.json
manually, and I fear that adding a "Custom" option may clutter the UI for the other users who, I think, are in vast majority here.
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.
I left some additional comments please adres them before merging
return false; | ||
} | ||
|
||
return !("build" in obj) || obj.build === undefined || isEasBuildConfig(obj.build); |
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.
this is elaborate would you mind commenting why this check is "correct"
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.
It's just checking if the shape of the object matches what we expect it to be -- is it really that puzzling?
I suppose you might want a reference to some schema here for the eas.json
file -- previously this was in the same place that read that file, so the context was there to see that's what it's doing, but I suppose now that it's an exported part of a module, some comment might be useful.
return false; | ||
} | ||
|
||
return Object.values(obj).every((v) => v === undefined || isEasBuildProfile(v)); |
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.
again would you mind adding some comments linking to eas documentation or explaining this checks?
Adds options to the "Launch Configuration" GUI for enabling EAS Build, selecting build profiles and specifying build UUIDs.
How Has This Been Tested:
expo-52-eas
and open the "Launch Configuration" windowlaunch.config
file appropriatelybuild
config ineas.json
(for example,react-native-77
or remove thebuild
entry ineas.json
inexpo-52-eas
) and open the "Launch Configuration" window