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

[eas-cli] use expo config related functions shipped with project's expo package to resolve config #2529

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

szdziedzic
Copy link
Member

@szdziedzic szdziedzic commented Sep 2, 2024

Why

https://exponent-internal.slack.com/archives/C02123T524U/p1724970663050089?thread_ts=1724924614.773089&cid=C02123T524U
https://exponent-internal.slack.com/archives/C5ERY0TAR/p1725002816077089

Generally @expo/config, @expo/prebuild-config, and @expo/config-plugins are per SDK concept. Currently, in EAS CLI, we rely on one version of the @expo/... package to work well globally for all SDKs. It seems to sometimes bite us like in the case of https://exponent-internal.slack.com/archives/C02123T524U/p1724909908574979, because SDK 51 config wasn't compatible with SDK 50.

It's better to switch to using config-related functions shipped with @expo/config, @expo/prebuild-config, and @expo/config-plugins. I believe we can assume that getConfig, getPrebuildConfig, and compileModsAsync shipped with these packages have stable API https://exponent-internal.slack.com/archives/C5ERY0TAR/p1725293002128849?thread_ts=1725002816.077089&cid=C5ERY0TAR.

How

Use getConfig, getPrebuildConfig, and compileModsAsync from @expo/config, @expo/prebuild-config, and @expo/config-plugins shipped with Expo SDK installed in the user's project. If not available, fallback to versions installed per SDK. If we can assume that these guarantee stable API we can use types of these functions from packages installed for EAS CLI as well.

Test Plan

Run builds for repro case of https://exponent-internal.slack.com/archives/C02123T524U/p1724909908574979 with SDK 51 config packages installed for EAS CLI (that previously broke it), see that it works and capabilities are correctly synced.

Initiate a new bare RN project using community CLI, and check that it works as well using fallback config loading.

Copy link
Member Author

szdziedzic commented Sep 2, 2024

Copy link

github-actions bot commented Sep 2, 2024

Size Change: -1.57 kB (0%)

Total Size: 52.9 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 52.9 MB -1.57 kB (0%)

compressed-size-action

Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 53.75000% with 37 lines in your changes missing coverage. Please review.

Project coverage is 53.10%. Comparing base (2dbaed5) to head (8536a7a).

Files with missing lines Patch % Lines
packages/eas-cli/src/project/ios/entitlements.ts 30.77% 9 Missing ⚠️
packages/eas-cli/src/credentials/context.ts 14.29% 6 Missing ⚠️
packages/eas-cli/src/project/expoConfig.ts 76.00% 6 Missing ⚠️
...dUtils/context/DynamicProjectConfigContextField.ts 0.00% 4 Missing ⚠️
...kages/eas-cli/src/credentials/manager/ManageIos.ts 0.00% 3 Missing ⚠️
...ontext/OptionalPrivateProjectConfigContextField.ts 33.34% 2 Missing ⚠️
...dUtils/context/PrivateProjectConfigContextField.ts 33.34% 2 Missing ⚠️
...ackages/eas-cli/src/commands/project/onboarding.ts 33.34% 2 Missing ⚠️
packages/eas-cli/src/project/projectUtils.ts 33.34% 2 Missing ⚠️
...rc/credentials/manager/SetUpIosBuildCredentials.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2529      +/-   ##
==========================================
- Coverage   53.12%   53.10%   -0.01%     
==========================================
  Files         564      564              
  Lines       21474    21508      +34     
  Branches     4198     4207       +9     
==========================================
+ Hits        11405    11420      +15     
- Misses      10039    10058      +19     
  Partials       30       30              

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

@szdziedzic szdziedzic changed the title [eas-cli] use getConfig from projectDir/node_modules/@expo/config if available [eas-cli] use expo config related functions shipped with project's expo package to resolve config Sep 2, 2024
@szdziedzic szdziedzic marked this pull request as ready for review September 2, 2024 16:29
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.

There are two approaches we could take, I don't have a strong preference:

  1. Call the CLI (npx expo config).
  2. require these modules in the project (what this PR does).

For expo-updates calls from EAS CLI we chose to use a CLI that returns JSON since it's a bit clearer to reviewers when a change is a breaking change, and as developers we're more attuned to knowing when a CLI change is breaking.

For option 2 to be stable in perpetuity, we need to add some constraint/test/something in the @expo/config package that calls it in the same way that we'd be introducing here in this PR to ensure that the function signature never changes. My recommendation would be an integration test in that package that is essentially the same as the updated expoConfig.ts and entitlements.ts files in this CLI.

I think if we can do option 2 (what this PR does) and add a signature stability API then this PR is good to go, but let's add that stability guarantee to the destination package before landing this to be sure it's doable.

Copy link
Member

Just a new piece of info: npx expo config has a --json flag to disable console during execution to prevent broken JSON!

Copy link
Member Author

Awesome! I will refactor it to use approach number 1 as in expo-updates then. Thanks @wschurman.

Copy link
Member Author

Oh, I see that it can be pretty straightforward for npx expo config however there still is compileModsAsync 🤔 I believe it still needs to be dynamically required, right? 🤔

@szdziedzic szdziedzic force-pushed the 09-02-_eas-cli_remove_expo-config-types_dependency branch from 82304e9 to 016c941 Compare October 14, 2024 11:35
@szdziedzic szdziedzic force-pushed the 09-02-_eas-cli_use_getconfig_from_projectdir_node_modules__expo_config_if_available branch from b5f49ac to 9cd41f7 Compare October 14, 2024 11:35
Copy link

github-actions bot commented Oct 14, 2024

CodeMention:

File Patterns Mentions
**/* @szdziedzic, @khamilowicz, @sjchmiela
packages/eas-cli/src/commands/update/** @EvanBacon, @byCedric, @wschurman

Copy link
Member Author

Ok I believe npx config --type introspect should do the job here 🎉

Copy link
Member Author

@wschurman Thanks once again for the tips! I'm sorry that it took so long I was off the grid for the last 2 weeks and I've just got back to working normally. Can you please take a look at it again 🙏?

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.

One inline comment, otherwise LGTM. As far as releasing this goes, we'll definitely want to do some testing of a broad set of commands to ensure it works since almost every command calls it.

['expo', 'config', '--json', ...(opts.isPublicConfig ? ['--type', 'public'] : [])],

{
cwd: projectDir,
Copy link
Member

Choose a reason for hiding this comment

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

do we need to pass env into spawnAsync? (we do for the expo-updates CLI). Or does the process.env override on L52 work?

Copy link
Member

Would also be good to get one more approval on this before landing. Could help to remove some reviewers to better indicate who is responsible for review.

@szdziedzic szdziedzic removed the request for review from EvanBacon October 17, 2024 12:06
@szdziedzic szdziedzic changed the base branch from 09-02-_eas-cli_remove_expo-config-types_dependency to graphite-base/2529 October 17, 2024 12:56
@szdziedzic szdziedzic force-pushed the 09-02-_eas-cli_use_getconfig_from_projectdir_node_modules__expo_config_if_available branch from 0dd8947 to da4d277 Compare October 17, 2024 12:57
@szdziedzic szdziedzic changed the base branch from graphite-base/2529 to main October 17, 2024 12:57
@szdziedzic szdziedzic force-pushed the 09-02-_eas-cli_use_getconfig_from_projectdir_node_modules__expo_config_if_available branch from da4d277 to 8536a7a Compare October 17, 2024 12:57
Copy link

❌ It looks like a changelog entry is missing for this PR. Add it manually to CHANGELOG.md.
⏩ If this PR doesn't require a changelog entry, such as if it's an internal change that doesn't affect the user experience, you can add the "no changelog" label to the PR.

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.

2 participants