Skip to content

Commit

Permalink
chore: relax forbidden react server API errors (#27878)
Browse files Browse the repository at this point in the history
# Why

In RSC, components can be shared between server and client, the client
components may contain react hooks which will run on the client but
shouldn't run on the server. This PR reduces the errors to only throw
when APIs are used and not when they're simply imported. The import
errors still run when importing `Component` or `PureComponent`.

To fully support shared components, we'll need a follow up PR to
"optimize" server components. This should collapse unused hooks so that
they don't run, and therefore don't trigger the static error pass. This
optimization pass is a bit tricky since we have old babel passes that
obfuscate the expected syntax, ref
facebook/react-native#43662

# Test Plan

- Updated the tests with the expected syntax.

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
  • Loading branch information
EvanBacon and expo-bot committed Apr 5, 2024
1 parent cdfd9c1 commit 96c7370
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 31 deletions.
1 change: 1 addition & 0 deletions packages/babel-preset-expo/CHANGELOG.md
Expand Up @@ -19,6 +19,7 @@

### 💡 Others

- Relax forbidden React server API errors to better support shared components. ([#27878](https://github.com/expo/expo/pull/27878) by [@EvanBacon](https://github.com/EvanBacon))
- Reset env in tests. ([#27950](https://github.com/expo/expo/pull/27950) by [@EvanBacon](https://github.com/EvanBacon))
- Add Hermes language support tests. ([#27900](https://github.com/expo/expo/pull/27900) by [@EvanBacon](https://github.com/EvanBacon))
- Remove unused peer dependency on `@babel/preset-env`. ([#27705](https://github.com/expo/expo/pull/27705) by [@EvanBacon](https://github.com/EvanBacon))
Expand Down
26 changes: 24 additions & 2 deletions packages/babel-preset-expo/build/restricted-react-api-plugin.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

This file was deleted.

Expand Up @@ -22,7 +22,7 @@ const DEF_OPTIONS = {
filename: '/unknown',

babelrc: false,
presets: [[preset, { disableImportExportTransform: true }]],
presets: [[preset, {}]],
sourceMaps: true,
configFile: false,
compact: false,
Expand All @@ -49,12 +49,13 @@ function getOpts(caller: Record<string, string | boolean>) {
}

describe('forbidden React server APIs', () => {
function runServerPass(src: string) {
function runServerPass(src: string, options = {}) {
return babel.transform(
src,
getOpts({
isReactServer: true,
platform: 'ios',
...options,
})
);
}
Expand All @@ -64,26 +65,33 @@ describe('forbidden React server APIs', () => {
'react-server-dom-webpack'
);
});
it(`does not assert importing client-side React functions in server components`, () => {
runServerPass(`import { useState } from 'react';`);
runServerPass(`import { useRef, useContext } from 'react';`);
runServerPass(`import React, { useRef } from 'react';`);
runServerPass(`import { useRandom } from 'react';`);
});
it(`asserts importing client-side React APIs in server components`, () => {
expect(() => runServerPass(`import { useState } from 'react';`)).toThrowErrorMatchingSnapshot();
expect(() => runServerPass(`import { useRef, useContext } from 'react';`)).toThrowError();
expect(() => runServerPass(`import React, { useRef } from 'react';`)).toThrowError();
expect(() => runServerPass(`import React, { PureComponent } from 'react';`)).toThrowError();
expect(() => runServerPass(`import { PureComponent } from 'react';`)).toThrowError();
expect(() => runServerPass(`import { Component } from 'react';`)).toThrowError();
expect(() => runServerPass(`import { useRandom } from 'react';`)).not.toThrowError();
});
it(`asserts importing client-side react-dom APIs in server components`, () => {
expect(() =>
runServerPass(`import { findDOMNode } from 'react-dom';`)
).toThrowErrorMatchingSnapshot();
expect(() => runServerPass(`import { useRandom } from 'react-dom';`)).not.toThrowError();
// Support importing but not using. This allows for shared components to import React APIs and only have assertions on usage.
it(`does not assert importing client-side react-dom APIs in server components`, () => {
runServerPass(`import { findDOMNode } from 'react-dom';`);
runServerPass(`import { useRandom } from 'react-dom';`);
});
it(`does not assert importing client-side react-dom APIs in server components if they are in node modules`, () => {
expect(
babel.transform(`import { findDOMNode } from 'react-dom';`, {
...DEF_OPTIONS,
filename: '/bacon/node_modules/@bacons/breakfast.js',
caller: getCaller({ ...ENABLED_CALLER, isReactServer: true, platform: 'ios' }),
caller: getCaller({
...ENABLED_CALLER,
supportsStaticESM: true,
isReactServer: true,
platform: 'ios',
}),
})?.code
).toBe(`import { findDOMNode } from 'react-dom';`);
});
Expand All @@ -100,6 +108,32 @@ describe('forbidden React server APIs', () => {
`)
).toThrow(/cannot be used in a React server component/);
});
it(`asserts if client-side React APIs are used in server components from a named import`, () => {
expect(() =>
runServerPass(`
import { useState } from 'react';
export default function App() {
const [index, setIndex] = useState(0)
return <div>{index}</div>;
}
`)
).toThrow(/cannot be used in a React server component/);
});
it(`does not asserts client-side code without React API usage in server components (named import)`, () => {
expect(() =>
runServerPass(`
import { useState } from 'react';
export default function App() {
const [index, setIndex] = foo()
return <div>{index}</div>;
}
function foo() {}
`)
).not.toThrow();
});
it(`asserts client-side React API usage in server components (default import)`, () => {
expect(() =>
runServerPass(`
Expand Down
37 changes: 33 additions & 4 deletions packages/babel-preset-expo/src/restricted-react-api-plugin.ts
Expand Up @@ -67,10 +67,22 @@ export function environmentRestrictedReactAPIsPlugin(
const isForbidden = forbiddenList.includes(importName);

if (isForbidden) {
// Add special handling for `Component` since it is different to a function API.
throw path.buildCodeFrameError(
`Client-only "${sourceValue}" API "${importName}" cannot be imported in a React server component. Add the "use client" directive to the top of this file or one of the parent files to enable running this stateful code on a user's device.`
);
if (['Component', 'PureComponent'].includes(importName)) {
// Add special handling for `Component` since it is different to a function API.
throw path.buildCodeFrameError(
`Client-only "${sourceValue}" API "${importName}" cannot be imported in a React server component. Add the "use client" directive to the top of this file or one of the parent files to enable running this stateful code on a user's device.`
);
} else {
const forbiddenImports: Map<string, Set<string>> = path.scope.getData(
'forbiddenImports'
) ?? new Map();

if (!forbiddenImports.has(sourceValue))
forbiddenImports.set(sourceValue, new Set());
forbiddenImports.get(sourceValue)!.add(importName);

path.scope.setData('forbiddenImports', forbiddenImports);
}
}
} else {
const importName = t.isStringLiteral(specifier.local)
Expand All @@ -83,6 +95,23 @@ export function environmentRestrictedReactAPIsPlugin(
});
}
},

// Match against `var _useState = useState(0),`
VariableDeclarator(path) {
const importedSpecifiers: undefined | Map<string, Set<string>> =
path.scope.getData('forbiddenImports');
if (!importedSpecifiers) return;
importedSpecifiers.forEach((forbiddenApis, importName) => {
if (t.isCallExpression(path.node.init) && t.isIdentifier(path.node.init.callee)) {
if (forbiddenApis.has(path.node.init.callee.name)) {
throw path.buildCodeFrameError(
`Client-only "useState" API cannot be used in a React server component. Add the "use client" directive to the top of this file or one of the parent files to enable running this stateful code on a user's device.`
);
}
}
});
},

MemberExpression(path) {
const importedNamespaces = path.scope.getData('importedNamespace') || {};
Object.keys(importedNamespaces).forEach((namespace) => {
Expand Down

0 comments on commit 96c7370

Please sign in to comment.