Skip to content
This repository has been archived by the owner on Jan 18, 2024. It is now read-only.

[doctor] check for sdkVersion in Expo config #4732

Merged
merged 4 commits into from
Jul 20, 2023

Conversation

keith-kurak
Copy link
Contributor

@keith-kurak keith-kurak commented Jul 7, 2023

Why

Detects when someone's upgrade goes awry due to sdkVersion in Expo config overriding the actual installed SDK version. (e.g., expo/expo#23351)

How

We can't use exp from getExpoConfig() because this always populates sdkVersion (either based on installed version or expo.sdkVersion). So, we have to read the actual Expo config to see if sdkVersion was set. This is really only practical in Doctor for a static config file. We could possibly attempt some comparison between exp's sdkVersion and package.json, but there'd be limitations to have to tiptoe around there.

One other option would be to actually change @expo/config to expose where sdkVersion came from. Not sure if it's worth the effort, though.

@brentvatne I can add some unit tests to this, but wanted to first run the concept by you first to see if we should take this approach or go further in one of the directions described above before I spent more time on it.

On Brent's advice, we now compare the final output from getConfig in @expo/config with how @expo/config resolves the installed SDK version. If they don't match, sdkVersion is being overridden in the Expo config, whether it's static or dynamic.

Test Plan

The message itself:
image

When you put in the wrong sdkVersion, you also get a ton of wrong package versions from the step where it checks against bundledNativeModules (not pictured)

@keith-kurak keith-kurak marked this pull request as ready for review July 7, 2023 21:45
* version of the `expo` package.
* Adapted from https://github.com/expo/expo/blob/main/packages/%40expo/config/src/getExpoSDKVersion.ts
*/
function getExpoSDKVersionFromPackage(projectRoot: string): string | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we can ask @expo/config? If so, I think that might be a better bet. The main reason for that is that we probably want @expo/config to handle all configuration loading, including the app manifest.

When we change something in a future version of @expo/config, we also need to make that change here. That is, unless we use the installed @expo/config package from the project and just use that here.

I do the same for expo-github-action, where I pull the configuration file, required to retrieve basic information for the update / preview sub-action. (see this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can get sdkVersion from Expo config, but not where the sdkVersion came from (Expo config or installed dependency). We use the sdkVersion from @expo/config everywhere else, but here, the idea is to check if someone overrode it in their Expo config (which in turn can usually break your build and throw off most other doctor checks). This is copied from @expo/config, but it's really just checking the version of the expo package installed, so I don't think it would change often.

Copy link
Member

@byCedric byCedric left a comment

Choose a reason for hiding this comment

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

LGTM

@keith-kurak keith-kurak merged commit b1852a8 into main Jul 20, 2023
0 of 4 checks passed
@keith-kurak keith-kurak deleted the keith/doctor-add-sdkVersion-check branch July 20, 2023 13:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants