Skip to content

Commit

Permalink
fix(cli): resolve proper versions with install expo@canary --fix (#…
Browse files Browse the repository at this point in the history
…25702)

# Why

This is a follow-up of #25600

# How

This PR addresses a couple of things:

- `expo install <package> --fix` should only fix that package. But,
`expo` affects all other packages, and should be handled properly when
executing `expo install expo@canary --fix`.
- Some package managers might use `"canary"` (dist tag) when installing
through `expo install expo@canary`, meaning the canary detection from
#25600 was not good enough.
- Move the canary detection into `getCombinedKnownVersionsAsync` to
include this in `expo install --fix` _**and**_ `expo install <package>`.

# Test Plan

- `$ bun create expo ./test-canary --template blank`
- `$ bun expo install expo@canary --fix`
- _should result in: `expo@canary` (or exact version),
`expo-status-bar@0.0.x-canary-...`_
- `$ bun expo install expo-dev-client`
  - _should result in: `expo-dev-client@0.0.x-canary-...`_
- `$ bun expo install --check` -> _should show no errors_

There were edge cases, because this is a new concept and I can't really
test this properly (it invokes the canary CLI, not CLI from source).
Because of that, this is now not allowed:

- `$ bun expo install expo@canary expo-dev-client --fix`

The command above not only changes `expo@canary,` but it also invokes
`expo install expo-dev-client --fix,` which throws because it's not yet
installed.

To avoid some confusion, this combination is aborted early without
installing anything.

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
  • Loading branch information
byCedric committed Dec 2, 2023
1 parent 5ebc8bd commit b01b873
Show file tree
Hide file tree
Showing 8 changed files with 321 additions and 102 deletions.
30 changes: 28 additions & 2 deletions packages/@expo/cli/e2e/__tests__/install-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ it(
'runs `npx expo install --check` fails',
async () => {
const projectRoot = await setupTestProjectAsync('install-check-fail', 'with-blank');
await installAsync(projectRoot, ['add', 'expo-sms@1.0.0', 'expo-auth-session@1.0.0']);
await installAsync(projectRoot, ['expo-sms@1.0.0', 'expo-auth-session@1.0.0']);

let pkg = await JsonFile.readAsync(path.resolve(projectRoot, 'package.json'));
// Added expected package
Expand Down Expand Up @@ -147,7 +147,7 @@ it(
'runs `npx expo install --fix` fails',
async () => {
const projectRoot = await setupTestProjectAsync('install-fix-fail', 'with-blank');
await installAsync(projectRoot, ['add', 'expo-sms@1.0.0', 'expo-auth-session@1.0.0']);
await installAsync(projectRoot, ['expo-sms@1.0.0', 'expo-auth-session@1.0.0']);

await execa('node', [bin, 'install', '--fix', 'expo-sms'], { cwd: projectRoot });

Expand Down Expand Up @@ -178,3 +178,29 @@ it(
// Could take 45s depending on how fast npm installs
60 * 1000
);

it(
'runs `npx expo install expo@<version> --fix`',
async () => {
const projectRoot = await setupTestProjectAsync('install-expo-canary-fix', 'with-blank');
const pkg = new JsonFile(path.resolve(projectRoot, 'package.json'));

// Add a package that requires "fixing" when using canary
await execa('node', [bin, 'install', 'expo-dev-client'], { cwd: projectRoot });

// Ensure `expo-dev-client` is installed
expect(pkg.read().dependencies).toMatchObject({
'expo-dev-client': expect.any(String),
});

// Add `expo@canary` to the project, and `--fix` project dependencies
await execa('node', [bin, 'install', 'expo@canary', '--fix'], { cwd: projectRoot });

// Ensure `expo-dev-client` is using canary version
expect(pkg.read().dependencies).toMatchObject({
'expo-dev-client': expect.stringContaining('canary'),
});
},
// Could take 45s depending on how fast npm installs
60 * 1000
);
49 changes: 39 additions & 10 deletions packages/@expo/cli/src/install/installAsync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { installExpoPackageAsync } from './installExpoPackage';
import { Options } from './resolveOptions';
import * as Log from '../log';
import { getVersionedPackagesAsync } from '../start/doctor/dependencies/getVersionedPackages';
import { CommandError } from '../utils/errors';
import { findUpProjectRootOrAssert } from '../utils/findUp';
import { learnMore } from '../utils/link';
import { setNodeEnv } from '../utils/nodeEnv';
Expand Down Expand Up @@ -43,7 +44,19 @@ export async function installAsync(
log: Log.log,
});

if (options.check || options.fix) {
const expoVersion = findPackageByName(packages, 'expo');
const otherPackages = packages.filter((pkg) => pkg !== expoVersion);

// Abort early when installing `expo@<version>` and other packages with `--fix/--check`
if (packageHasVersion(expoVersion) && otherPackages.length && (options.check || options.fix)) {
throw new CommandError(
'BAD_ARGS',
`Cannot install other packages with ${expoVersion} and --fix or --check`
);
}

// Only check/fix packages if `expo@<version>` is not requested
if (!packageHasVersion(expoVersion) && (options.check || options.fix)) {
return await checkPackagesAsync(projectRoot, {
packages,
options,
Expand All @@ -61,6 +74,7 @@ export async function installAsync(

// Resolve the versioned packages, then install them.
return installPackagesAsync(projectRoot, {
...options,
packageManager,
packages,
packageManagerArguments,
Expand All @@ -76,7 +90,9 @@ export async function installPackagesAsync(
packageManager,
sdkVersion,
packageManagerArguments,
}: {
fix,
check,
}: Options & {
/**
* List of packages to version, grouped by the type of dependency.
* @example ['uuid', 'react-native-reanimated@latest']
Expand Down Expand Up @@ -157,22 +173,35 @@ export async function installPackagesAsync(
}
}

// if updating expo package, install this first, then re-run the command minus expo to install everything else
if (packages.find((pkg) => pkg === 'expo')) {
const packagesMinusExpo = packages.filter((pkg) => pkg !== 'expo');
// `expo` needs to be installed before installing other packages
const expoPackage = findPackageByName(packages, 'expo');
if (expoPackage) {
const postInstallCommand = packages.filter((pkg) => pkg !== expoPackage);

// Pipe options to the next command
if (fix) postInstallCommand.push('--fix');
if (check) postInstallCommand.push('--check');

await installExpoPackageAsync(projectRoot, {
// Abort after installing `expo`, follow up command is spawn in a new process
return await installExpoPackageAsync(projectRoot, {
packageManager,
packageManagerArguments,
expoPackageToInstall: versioning.packages.find((pkg) => pkg.startsWith('expo@'))!,
followUpCommandArgs: packagesMinusExpo,
followUpCommandArgs: postInstallCommand,
});

// follow-up commands will be spawned in a detached process, so return immediately
return;
}

await packageManager.addAsync([...packageManagerArguments, ...versioning.packages]);

await applyPluginsAsync(projectRoot, versioning.packages);
}

/** Find a package, by name, in the requested packages list (`expo` -> `expo`/`expo@<version>`) */
function findPackageByName(packages: string[], name: string) {
return packages.find((pkg) => pkg === name || pkg.startsWith(`${name}@`));
}

/** Determine if a specific version is requested for a package */
function packageHasVersion(name = '') {
return name.includes('@');
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
getRemoteVersionsForSdkAsync,
getVersionedPackagesAsync,
} from '../getVersionedPackages';
import { hasExpoCanaryAsync } from '../resolvePackages';

jest.mock('../../../../log');

Expand All @@ -18,6 +19,10 @@ jest.mock('../bundledNativeModules', () => ({
getVersionedNativeModulesAsync: jest.fn(),
}));

jest.mock('../resolvePackages', () => ({
hasExpoCanaryAsync: jest.fn().mockResolvedValue(false),
}));

describe(getCombinedKnownVersionsAsync, () => {
it(`should prioritize remote versions over bundled versions`, async () => {
// Remote versions
Expand Down Expand Up @@ -45,7 +50,8 @@ describe(getCombinedKnownVersionsAsync, () => {
});
});

it(`skips fetching remote versions when requested`, async () => {
it(`skips remote versions for canary releases`, async () => {
jest.mocked(hasExpoCanaryAsync).mockResolvedValueOnce(true);
jest.mocked(getVersionedNativeModulesAsync).mockResolvedValue({
shared: 'bundled',
});
Expand All @@ -57,7 +63,6 @@ describe(getCombinedKnownVersionsAsync, () => {
await getCombinedKnownVersionsAsync({
projectRoot: '/',
sdkVersion: '1.0.0',
skipRemoteVersions: true,
})
).toEqual({
shared: 'bundled',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
import { vol } from 'memfs';
import resolveFrom from 'resolve-from';

import {
resolvePackageVersionAsync,
resolveAllPackageVersionsAsync,
hasExpoCanaryAsync,
} from '../resolvePackages';

afterEach(() => {
vol.reset();
});

const projectRoot = '/fake/project';

describe(resolvePackageVersionAsync, () => {
it('resolves installed package', async () => {
vol.fromJSON(
{ [`node_modules/expo/package.json`]: JSON.stringify({ version: '1.0.0' }) },
projectRoot
);

await expect(resolvePackageVersionAsync(projectRoot, 'expo')).resolves.toBe('1.0.0');
});

it('resolves installed package using `exports` without `package.json`', async () => {
// Mock the Node error when not exporting `package.json` from `exports`.
jest.mocked(resolveFrom).mockImplementationOnce(() => {
const error: any = new Error(
`Package subpath './package.json' is not defined by "exports" in ${projectRoot}/node_modules/expo/package.json`
);
error.code = 'ERR_PACKAGE_PATH_NOT_EXPORTED';
throw error;
});

vol.fromJSON(
{
[`node_modules/expo/package.json`]: JSON.stringify({
version: '2.0.0',
exports: {
'.': {
require: './index.js',
},
},
}),
},
projectRoot
);

await expect(resolvePackageVersionAsync(projectRoot, 'expo')).resolves.toBe('2.0.0');
});

it('throws when package is not installed', async () => {
vol.fromJSON({}, projectRoot);

await expect(resolvePackageVersionAsync(projectRoot, 'expo')).rejects.toThrowError(
`"expo" is added as a dependency in your project's package.json but it doesn't seem to be installed`
);
});
});

describe(resolveAllPackageVersionsAsync, () => {
it('resolves installed packages', async () => {
vol.fromJSON(
{
[`node_modules/expo/package.json`]: JSON.stringify({ version: '1.0.0' }),
[`node_modules/react/package.json`]: JSON.stringify({ version: '2.0.0' }),
},
projectRoot
);

await expect(
resolveAllPackageVersionsAsync(projectRoot, ['expo', 'react'])
).resolves.toMatchObject({
expo: '1.0.0',
react: '2.0.0',
});
});

it('throws when package is not installed', async () => {
vol.fromJSON(
{
[`node_modules/expo/package.json`]: JSON.stringify({ version: '1.0.0' }),
},
projectRoot
);

await expect(resolveAllPackageVersionsAsync(projectRoot, ['expo', 'react'])).rejects.toThrow(
`"react" is added as a dependency in your project's package.json but it doesn't seem to be installed`
);
});
});

describe(hasExpoCanaryAsync, () => {
it('returns false for stable version from installed package', async () => {
vol.fromJSON(
{
[`node_modules/expo/package.json`]: JSON.stringify({ version: '1.0.0' }),
// This is not a common use-case, but tests the priority of strategies
[`package.json`]: JSON.stringify({
version: '1.0.0',
dependencies: {
expo: 'canary',
},
}),
},
projectRoot
);

await expect(hasExpoCanaryAsync(projectRoot)).resolves.toBe(false);
});

it('returns true for canary version from installed package', async () => {
vol.fromJSON(
{
[`node_modules/expo/package.json`]: JSON.stringify({
version: '50.0.0-canary-20231130-c8a9bf9',
}),
// This is not a common use-case, but tests the priority of strategies
[`package.json`]: JSON.stringify({
version: '1.0.0',
dependencies: {
expo: '1.0.0',
},
}),
},
projectRoot
);

await expect(hasExpoCanaryAsync(projectRoot)).resolves.toBe(true);
});

it('returns false for stable version from package.json', async () => {
vol.fromJSON(
{
[`package.json`]: JSON.stringify({
version: '1.0.0',
dependencies: {
expo: '1.0.0',
},
}),
},
projectRoot
);

await expect(hasExpoCanaryAsync(projectRoot)).resolves.toBe(false);
});

it('returns true for canary version from package.json', async () => {
vol.fromJSON(
{
[`package.json`]: JSON.stringify({
version: '1.0.0',
dependencies: {
expo: 'canary',
},
}),
},
projectRoot
);

await expect(hasExpoCanaryAsync(projectRoot)).resolves.toBe(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import path from 'path';
import resolveFrom from 'resolve-from';

import * as Log from '../../../../log';
import { getCombinedKnownVersionsAsync } from '../getVersionedPackages';
import {
logIncorrectDependencies,
validateDependenciesVersionsAsync,
Expand Down Expand Up @@ -233,39 +232,6 @@ describe(validateDependenciesVersionsAsync, () => {
);
});

it('only uses local bundled module versions when using canary versions', async () => {
vol.fromJSON(
{
'node_modules/expo/package.json': JSON.stringify({
version: '50.0.0-canary-20231125-d600e44',
}),
'node_modules/react-native/package.json': JSON.stringify({
version: '0.73.0-rc.5',
}),
},
projectRoot
);
jest.mocked(getCombinedKnownVersionsAsync).mockResolvedValue({
'react-native': '0.73.0-rc.5',
});

const exp = { sdkVersion: '50.0.0' };
const pkg = { dependencies: { 'react-native': '0.73.0-rc.5' } };

// This should pass validation without any problems
await expect(validateDependenciesVersionsAsync(projectRoot, exp as any, pkg)).resolves.toBe(
true
);
// This should be called with `skipRemoteVersions: true` because of a `canary` Expo SDK version
expect(getCombinedKnownVersionsAsync).toBeCalledWith(
expect.objectContaining({
projectRoot,
sdkVersion: exp.sdkVersion,
skipRemoteVersions: true,
})
);
});

it('skips validating dependencies when running in offline mode', async () => {
jest.resetModules();

Expand Down

0 comments on commit b01b873

Please sign in to comment.