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

[ENG-14625][eas-cli] add eas build:dev command #2820

Merged
merged 8 commits into from
Jan 30, 2025

Conversation

szdziedzic
Copy link
Member

@szdziedzic szdziedzic commented Jan 16, 2025

Why

https://exponent-internal.slack.com/archives/C1QLM27T9/p1734729481930599?thread_ts=1734708689.473259&cid=C1QLM27T9

Add eas dev command that:

  1. calculates fingerprint
  2. checks if we have simulator/emulator dev client builds with such a fingerprint hash on our servers
    a. if we do - it downloads and launches them on the emulator/simulator using the same logic we use for eas build:run
    b. if not - such a build is created and launched afterward

How

Do exactly what I mentioned above. Use as much code and functions from build logic and eas build:run that already exist to make it easier to maintain.

Test Plan

Test manually

https://exponent-internal.slack.com/archives/C1QLM27T9/p1736987685783579?thread_ts=1734708689.473259&cid=C1QLM27T9

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@szdziedzic szdziedzic marked this pull request as ready for review January 16, 2025 00:34
Copy link

Subscribed to pull request

File Patterns Mentions
**/* @khamilowicz, @sjchmiela, @radoslawkrzemien

Generated by CodeMention

Copy link

github-actions bot commented Jan 16, 2025

Size Change: -289 B (0%)

Total Size: 53.4 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 53.4 MB -289 B (0%)

compressed-size-action

@brentvatne
Copy link
Member

brentvatne commented Jan 16, 2025

my main concern here is implementing this as a top level command, which feels like something we'd be committing to - and we may want to use the eas dev command for something else, that maybe does more than what we're doing here (perhaps in addition to this functionality). what if this was eas build:run --dev for now? if that was how to access this functionality i'd be good with rolling it out right away

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 20.58824% with 108 lines in your changes missing coverage. Please review.

Project coverage is 52.54%. Comparing base (3159550) to head (b82e878).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/eas-cli/src/commands/build/dev.ts 25.27% 64 Missing and 7 partials ⚠️
packages/eas-cli/src/build/configure.ts 11.54% 21 Missing and 2 partials ⚠️
packages/eas-cli/src/build/runBuildAndSubmit.ts 9.10% 10 Missing ⚠️
packages/eas-cli/src/commands/build/index.ts 0.00% 1 Missing ⚠️
packages/eas-cli/src/commands/build/inspect.ts 0.00% 1 Missing ⚠️
packages/eas-cli/src/commands/build/internal.ts 0.00% 1 Missing ⚠️
...ackages/eas-cli/src/commands/project/onboarding.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2820      +/-   ##
==========================================
- Coverage   52.71%   52.54%   -0.16%     
==========================================
  Files         588      589       +1     
  Lines       22912    23036     +124     
  Branches     4782     4814      +32     
==========================================
+ Hits        12076    12103      +27     
- Misses       9882     9970      +88     
- Partials      954      963       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@szdziedzic
Copy link
Member Author

what if this was eas build:run --dev for now? if that was how to access this functionality i'd be good with rolling it out right away

@brentvatne What do you think about eas build:dev?

@szdziedzic szdziedzic changed the title [eas-cli] add eas dev command [eas-cli] add eas build:dev command Jan 16, 2025
@szdziedzic szdziedzic changed the title [eas-cli] add eas build:dev command [ENG-14625][eas-cli] add eas build:dev command Jan 16, 2025
Copy link

linear bot commented Jan 16, 2025

@szdziedzic szdziedzic requested a review from brentvatne January 16, 2025 11:46
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

Few minor comments and one question about the build profile. Otherwise looks good to me. Will delegate to a reviewer more familiar with builds for approval though.

packages/eas-cli/src/commands/build/dev.ts Outdated Show resolved Hide resolved
packages/eas-cli/src/commands/build/dev.ts Outdated Show resolved Hide resolved
projectDir,
});
const buildProfiles =
buildProfilesOverride ??
Copy link
Member

Choose a reason for hiding this comment

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

Just curious (I'm not super familiar with build profiles), from a UX perspective should we require them to specify/choose an existing dev-client-enabled profile instead of creating a virtual profile for them? If not, are there any side effects of creating this "virtual" profile for them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think what you suggest here might be a better option 🤔 The Current implementation should "magically" work well out of the box for users, but I think it limits the customization of things (like selecting the bun version). I believe that using explicit profiles can also have the benefit of people having a good idea about what settings we apply under the hood for these builds 🤔 What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Definitely a good question for the reviewers on this who are more familiar with build profiles. I unfortunately only have a very surface-level understanding of them. My comment was spurred by the more general idea that having "virutal"/"synthesized" things often does lead to confusion as you mentioned.

packages/eas-cli/src/build/runBuildAndSubmit.ts Outdated Show resolved Hide resolved
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

Will delegate final approval to other reviewers more familiar with build.

projectDir,
});
const buildProfiles =
buildProfilesOverride ??
Copy link
Member

Choose a reason for hiding this comment

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

Definitely a good question for the reviewers on this who are more familiar with build profiles. I unfortunately only have a very surface-level understanding of them. My comment was spurred by the more general idea that having "virutal"/"synthesized" things often does lead to confusion as you mentioned.

packages/eas-cli/src/commands/build/dev.ts Outdated Show resolved Hide resolved
@szdziedzic szdziedzic force-pushed the 01-16-_eas-cli_add_eas_dev_command branch from 5e655a5 to 58e0389 Compare January 30, 2025 15:25
@szdziedzic szdziedzic added the no changelog PR that doesn't require a changelog entry label Jan 30, 2025
Copy link

⏩ The changelog entry check has been skipped since the "no changelog" label is present.

@szdziedzic szdziedzic merged commit 061f626 into main Jan 30, 2025
11 of 12 checks passed
@szdziedzic szdziedzic deleted the 01-16-_eas-cli_add_eas_dev_command branch January 30, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PR that doesn't require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants