Skip to content

React compiler neverSkip #3

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ module.exports = {
files: ['packages/react-server-dom-webpack/**/*.js'],
globals: {
__webpack_chunk_load__: 'readonly',
__webpack_get_script_filename__: 'readonly',
__webpack_require__: 'readonly',
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,14 @@ export const EnvironmentConfigSchema = z.object({
* useMemo(() => { ... }, [...]);
*/
validateNoVoidUseMemo: z.boolean().default(false),

/**
* A list of function identifier names that should never be skipped by
* memoization/flattening optimizations. Calls to any identifier with a name
* in this list behave like the `use` operator in terms of never being
* skipped, and may be called conditionally.
*/
neverSkipFunctionName: z.array(z.string()).default([]),
});

export type EnvironmentConfig = z.infer<typeof EnvironmentConfigSchema>;
Expand Down
28 changes: 28 additions & 0 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1915,6 +1915,34 @@ export function isUseOperator(id: Identifier): boolean {
);
}

/**
* Treat any call with a neverSkip identifier name similar to the `use` operator:
* - It may be called conditionally.
* - It should never be skipped by memoization/flattening logic.
*/
export function isNeverSkipIdentifier(
env: Environment,
id: Identifier,
): boolean {
const list = env.config.neverSkipFunctionName;
if (list == null || list.length === 0) {
return false;
}
if (id.name != null && list.includes(id.name.value)) {
return true;
}
const loc = id.loc as any;
if (
loc &&
typeof loc !== 'symbol' &&
typeof loc.identifierName === 'string' &&
list.includes(loc.identifierName)
) {
return true;
}
return false;
}

export function getHookKindForType(
env: Environment,
type: Type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
isStableType,
isStableTypeContainer,
isUseOperator,
isNeverSkipIdentifier,
} from '../HIR';
import {PostDominator} from '../HIR/Dominator';
import {
Expand Down Expand Up @@ -302,13 +303,15 @@ export function inferReactivePlaces(fn: HIRFunction): void {
if (
value.kind === 'CallExpression' &&
(getHookKind(fn.env, value.callee.identifier) != null ||
isUseOperator(value.callee.identifier))
isUseOperator(value.callee.identifier) ||
isNeverSkipIdentifier(fn.env, value.callee.identifier))
) {
hasReactiveInput = true;
} else if (
value.kind === 'MethodCall' &&
(getHookKind(fn.env, value.property.identifier) != null ||
isUseOperator(value.property.identifier))
isUseOperator(value.property.identifier) ||
isNeverSkipIdentifier(fn.env, value.property.identifier))
) {
hasReactiveInput = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
PrunedScopeTerminal,
getHookKind,
isUseOperator,
isNeverSkipIdentifier,
} from '../HIR';
import {retainWhere} from '../Utils/utils';

Expand Down Expand Up @@ -53,7 +54,8 @@ export function flattenScopesWithHooksOrUseHIR(fn: HIRFunction): void {
value.kind === 'MethodCall' ? value.property : value.callee;
if (
getHookKind(fn.env, callee.identifier) != null ||
isUseOperator(callee.identifier)
isUseOperator(callee.identifier) ||
isNeverSkipIdentifier(fn.env, callee.identifier)
) {
prune.push(...activeScopes.map(entry => entry.block));
activeScopes.length = 0;
Expand Down
1 change: 0 additions & 1 deletion compiler/scripts/release/shared/packages.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

const PUBLISHABLE_PACKAGES = [
'babel-plugin-react-compiler',
'eslint-plugin-react-compiler',
'react-compiler-healthcheck',
'react-compiler-runtime',
];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7735,6 +7735,9 @@ if (__EXPERIMENTAL__) {
useEffect(() => {
onStuff();
}, []);
React.useEffect(() => {
onStuff();
}, []);
}
`,
},
Expand All @@ -7751,6 +7754,9 @@ if (__EXPERIMENTAL__) {
useEffect(() => {
onStuff();
}, [onStuff]);
React.useEffect(() => {
onStuff();
}, [onStuff]);
}
`,
errors: [
Expand All @@ -7769,6 +7775,32 @@ if (__EXPERIMENTAL__) {
useEffect(() => {
onStuff();
}, []);
React.useEffect(() => {
onStuff();
}, [onStuff]);
}
`,
},
],
},
{
message:
'Functions returned from `useEffectEvent` must not be included in the dependency array. ' +
'Remove `onStuff` from the list.',
suggestions: [
{
desc: 'Remove the dependency `onStuff`',
output: normalizeIndent`
function MyComponent({ theme }) {
const onStuff = useEffectEvent(() => {
showNotification(theme);
});
useEffect(() => {
onStuff();
}, [onStuff]);
React.useEffect(() => {
onStuff();
}, []);
}
`,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1368,6 +1368,9 @@ if (__EXPERIMENTAL__) {
useEffect(() => {
onClick();
});
React.useEffect(() => {
onClick();
});
}
`,
},
Expand All @@ -1389,6 +1392,10 @@ if (__EXPERIMENTAL__) {
let id = setInterval(() => onClick(), 100);
return () => clearInterval(onClick);
}, []);
React.useEffect(() => {
let id = setInterval(() => onClick(), 100);
return () => clearInterval(onClick);
}, []);
return null;
}
`,
Expand All @@ -1408,13 +1415,17 @@ if (__EXPERIMENTAL__) {
{
code: normalizeIndent`
function MyComponent({ theme }) {
// Can receive arguments
const onEvent = useEffectEvent((text) => {
console.log(text);
});

useEffect(() => {
onEvent('Hello world');
});
React.useEffect(() => {
onEvent('Hello world');
});
}
`,
},
Expand Down
28 changes: 25 additions & 3 deletions packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import type {
CallExpression,
CatchClause,
DoWhileStatement,
Expression,
Identifier,
Node,
Super,
TryStatement,
} from 'estree';

Expand Down Expand Up @@ -129,6 +132,24 @@ function isInsideTryCatch(
return false;
}

function getNodeWithoutReactNamespace(
node: Expression | Super,
): Expression | Identifier | Super {
if (
node.type === 'MemberExpression' &&
node.object.type === 'Identifier' &&
node.object.name === 'React' &&
node.property.type === 'Identifier' &&
!node.computed
) {
return node.property;
}
return node;
}

function isUseEffectIdentifier(node: Node): boolean {
return node.type === 'Identifier' && node.name === 'useEffect';
}
function isUseEffectEventIdentifier(node: Node): boolean {
if (__EXPERIMENTAL__) {
return node.type === 'Identifier' && node.name === 'useEffectEvent';
Expand Down Expand Up @@ -702,10 +723,11 @@ const rule = {

// useEffectEvent: useEffectEvent functions can be passed by reference within useEffect as well as in
// another useEffectEvent
// Check all `useEffect` and `React.useEffect`, `useEffectEvent`, and `React.useEffectEvent`
const nodeWithoutNamespace = getNodeWithoutReactNamespace(node.callee);
if (
node.callee.type === 'Identifier' &&
(node.callee.name === 'useEffect' ||
isUseEffectEventIdentifier(node.callee)) &&
(isUseEffectIdentifier(nodeWithoutNamespace) ||
isUseEffectEventIdentifier(nodeWithoutNamespace)) &&
node.arguments.length > 0
) {
// Denote that we have traversed into a useEffect call, and stash the CallExpr for
Expand Down
23 changes: 19 additions & 4 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import {
resolveServerReference,
preloadModule,
requireModule,
getModuleDebugInfo,
dispatchHint,
readPartialStringChunk,
readFinalStringChunk,
Expand Down Expand Up @@ -790,8 +791,14 @@ function resolveModuleChunk<T>(
resolvedChunk.status = RESOLVED_MODULE;
resolvedChunk.value = value;
if (__DEV__) {
// We don't expect to have any debug info for this row.
resolvedChunk._debugInfo = null;
const debugInfo = getModuleDebugInfo(value);
if (debugInfo !== null && resolvedChunk._debugInfo != null) {
// Add to the live set if it was already initialized.
// $FlowFixMe[method-unbinding]
resolvedChunk._debugInfo.push.apply(resolvedChunk._debugInfo, debugInfo);
} else {
resolvedChunk._debugInfo = debugInfo;
}
}
if (resolveListeners !== null) {
initializeModuleChunk(resolvedChunk);
Expand Down Expand Up @@ -3977,7 +3984,11 @@ function flushComponentPerformance(
// Track the root most component of the result for deduping logging.
result.component = componentInfo;
isLastComponent = false;
} else if (candidateInfo.awaited) {
} else if (
candidateInfo.awaited &&
// Skip awaits on client resources since they didn't block the server component.
candidateInfo.awaited.env != null
) {
if (endTime > childrenEndTime) {
childrenEndTime = endTime;
}
Expand Down Expand Up @@ -4059,7 +4070,11 @@ function flushComponentPerformance(
// Track the root most component of the result for deduping logging.
result.component = componentInfo;
isLastComponent = false;
} else if (candidateInfo.awaited) {
} else if (
candidateInfo.awaited &&
// Skip awaits on client resources since they didn't block the server component.
candidateInfo.awaited.env != null
) {
// If we don't have an end time for an await, that means we aborted.
const asyncInfo: ReactAsyncInfo = candidateInfo;
const env = response._rootEnvironmentName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export const resolveClientReference = $$$config.resolveClientReference;
export const resolveServerReference = $$$config.resolveServerReference;
export const preloadModule = $$$config.preloadModule;
export const requireModule = $$$config.requireModule;
export const getModuleDebugInfo = $$$config.getModuleDebugInfo;
export const dispatchHint = $$$config.dispatchHint;
export const prepareDestinationForModule =
$$$config.prepareDestinationForModule;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@ export const resolveClientReference: any = null;
export const resolveServerReference: any = null;
export const preloadModule: any = null;
export const requireModule: any = null;
export const getModuleDebugInfo: any = null;
export const prepareDestinationForModule: any = null;
export const usedWithSSR = true;
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const resolveClientReference: any = null;
export const resolveServerReference: any = null;
export const preloadModule: any = null;
export const requireModule: any = null;
export const getModuleDebugInfo: any = null;
export const dispatchHint: any = null;
export const prepareDestinationForModule: any = null;
export const usedWithSSR = true;
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ export function requireModule<T>(metadata: ClientReference<T>): T {
);
}

export function getModuleDebugInfo<T>(metadata: ClientReference<T>): null {
throw new Error(
'renderToHTML should not have emitted Client References. This is a bug in React.',
);
}

export const usedWithSSR = true;

type HintCode = string;
Expand Down
Loading
Loading