Skip to content

Commit

Permalink
compiler: only resolve globals and react imports
Browse files Browse the repository at this point in the history
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc.

Still working on more tests.

ghstack-source-id: 0c26e76d41512db0976de16bd33475723d6b99f6
Pull Request resolved: #29190
  • Loading branch information
josephsavona committed May 21, 2024
1 parent 912f820 commit c93d4b3
Show file tree
Hide file tree
Showing 8 changed files with 392 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { fromZodError } from "zod-validation-error";
import { CompilerError } from "../CompilerError";
import { Logger } from "../Entrypoint";
import { Err, Ok, Result } from "../Utils/Result";
import { log } from "../Utils/logger";
import {
DEFAULT_GLOBALS,
DEFAULT_SHAPES,
Expand Down Expand Up @@ -320,6 +319,8 @@ const EnvironmentConfigSchema = z.object({
*/
throwUnknownException__testonly: z.boolean().default(false),

enableSharedRuntime__testonly: z.boolean().default(false),

/**
* Enables deps of a function epxression to be treated as conditional. This
* makes sure we don't load a dep when it's a property (to check if it has
Expand Down Expand Up @@ -514,7 +515,6 @@ export class Environment {

getGlobalDeclaration(binding: NonLocalBinding): Global | null {
const name = binding.name;
let resolvedName = name;

if (this.config.hookPattern != null) {
const match = new RegExp(this.config.hookPattern).exec(name);
Expand All @@ -523,21 +523,45 @@ export class Environment {
typeof match[1] === "string" &&
isHookName(match[1])
) {
resolvedName = match[1];
const resolvedName = match[1];
return this.#globals.get(resolvedName) ?? this.#getCustomHookType();
}
}

let resolvedGlobal: Global | null = this.#globals.get(resolvedName) ?? null;
if (resolvedGlobal === null) {
// Hack, since we don't track module level declarations and imports
if (isHookName(resolvedName)) {
return this.#getCustomHookType();
} else {
log(() => `Undefined global \`${name}\``);
let global: Global | null = null;
switch (binding.kind) {
case "ModuleLocal": {
// don't resolve module locals
break;
}
case "Global": {
global = this.#globals.get(name) ?? null;
break;
}
case "ImportDefault":
case "ImportNamespace":
case "ImportSpecifier": {
if (
binding.module.toLowerCase() === "react" ||
binding.module.toLowerCase() === "react-dom" ||
(this.config.enableSharedRuntime__testonly &&
binding.module === "shared-runtime")
) {
// only resolve imports to modules we know about
global = this.#globals.get(name) ?? null;
}
break;
}
}

return resolvedGlobal;
if (global === null && isHookName(name)) {
/**
* Type inference relies on all hooks being resolved as such, so if we don't have
* a global declaration and its a hook name, return the default custom hook type.
*/
return this.#getCustomHookType();
}
return global;
}

getPropertyType(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,38 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
break;
}
case "LoadGlobal": {
value = `LoadGlobal ${instrValue.binding.name}`;
switch (instrValue.binding.kind) {
case "Global": {
value = `LoadGlobal(global) ${instrValue.binding.name}`;
break;
}
case "ModuleLocal": {
value = `LoadGlobal(module) ${instrValue.binding.name}`;
break;
}
case "ImportDefault": {
value = `LoadGlobal import ${instrValue.binding.name} from '${instrValue.binding.module}'`;
break;
}
case "ImportNamespace": {
value = `LoadGlobal import * as ${instrValue.binding.name} from '${instrValue.binding.module}'`;
break;
}
case "ImportSpecifier": {
if (instrValue.binding.imported !== instrValue.binding.name) {
value = `LoadGlobal import { ${instrValue.binding.imported} as ${instrValue.binding.name} } from '${instrValue.binding.module}'`;
} else {
value = `LoadGlobal import { ${instrValue.binding.name} } from '${instrValue.binding.module}'`;
}
break;
}
default: {
assertExhaustive(
instrValue.binding,
`Unexpected binding kind \`${(instrValue.binding as any).kind}\``
);
}
}
break;
}
case "StoreGlobal": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,71 +56,78 @@ function useFragment(_arg1, _arg2) {
}

function Component(props) {
const $ = _c(14);
const post = useFragment(graphql`...`, props.post);
const $ = _c(15);
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = graphql`...`;
$[0] = t0;
} else {
t0 = $[0];
}
const post = useFragment(t0, props.post);
let media;
let allUrls;
let onClick;
if ($[0] !== post) {
if ($[1] !== post) {
allUrls = [];

const { media: t0, comments: t1, urls: t2 } = post;
media = t0 === undefined ? null : t0;
let t3;
if ($[4] !== t1) {
t3 = t1 === undefined ? [] : t1;
$[4] = t1;
$[5] = t3;
} else {
t3 = $[5];
}
const comments = t3;
const { media: t1, comments: t2, urls: t3 } = post;
media = t1 === undefined ? null : t1;
let t4;
if ($[6] !== t2) {
if ($[5] !== t2) {
t4 = t2 === undefined ? [] : t2;
$[6] = t2;
$[7] = t4;
$[5] = t2;
$[6] = t4;
} else {
t4 = $[7];
t4 = $[6];
}
const urls = t4;
const comments = t4;
let t5;
if ($[8] !== comments.length) {
t5 = (e) => {
if ($[7] !== t3) {
t5 = t3 === undefined ? [] : t3;
$[7] = t3;
$[8] = t5;
} else {
t5 = $[8];
}
const urls = t5;
let t6;
if ($[9] !== comments.length) {
t6 = (e) => {
if (!comments.length) {
return;
}

console.log(comments.length);
};
$[8] = comments.length;
$[9] = t5;
$[9] = comments.length;
$[10] = t6;
} else {
t5 = $[9];
t6 = $[10];
}
onClick = t5;
onClick = t6;

allUrls.push(...urls);
$[0] = post;
$[1] = media;
$[2] = allUrls;
$[3] = onClick;
$[1] = post;
$[2] = media;
$[3] = allUrls;
$[4] = onClick;
} else {
media = $[1];
allUrls = $[2];
onClick = $[3];
media = $[2];
allUrls = $[3];
onClick = $[4];
}
let t0;
if ($[10] !== media || $[11] !== allUrls || $[12] !== onClick) {
t0 = <Stringify media={media} allUrls={allUrls} onClick={onClick} />;
$[10] = media;
$[11] = allUrls;
$[12] = onClick;
$[13] = t0;
let t1;
if ($[11] !== media || $[12] !== allUrls || $[13] !== onClick) {
t1 = <Stringify media={media} allUrls={allUrls} onClick={onClick} />;
$[11] = media;
$[12] = allUrls;
$[13] = onClick;
$[14] = t1;
} else {
t0 = $[13];
t1 = $[14];
}
return t0;
return t1;
}

export const FIXTURE_ENTRYPOINT = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@

## Input

```javascript
import { useState as _useState, useCallback, useEffect } from "react";
import { ValidateMemoization } from "shared-runtime";

function useState(value) {
"use no memo"; // opt-out because we want to force resetting the setState function
const [state, _setState] = _useState(value);
const setState = useCallback(
(...args) => {
console.log(...args);
return _setState(...args);
},
// explicitly reset the callback when state changes
[state]
);
if (setState.state === undefined) {
setState.state = state;
}
return [state, setState];
}

function Component() {
const [state, setState] = useState("hello");
console.log(state, setState.state);

const callback = useCallback(() => {
setState("goodbye");
}, [setState]);

useEffect(() => {
callback();
}, []);

return (
<>
<ValidateMemoization inputs={[setState]} output={callback} />
{state}
</>
);
}

export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{}],
};

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime";
import { useState as _useState, useCallback, useEffect } from "react";
import { ValidateMemoization } from "shared-runtime";

function useState(value) {
"use no memo"; // opt-out because we want to force resetting the setState function
const [state, _setState] = _useState(value);
const setState = useCallback(
(...args) => {
console.log(...args);
return _setState(...args);
},
// explicitly reset the callback when state changes
[state]
);
if (setState.state === undefined) {
setState.state = state;
}
return [state, setState];
}

function Component() {
const $ = _c(13);
const [state, setState] = useState("hello");
console.log(state, setState.state);
let t0;
if ($[0] !== setState) {
t0 = () => {
setState("goodbye");
};
$[0] = setState;
$[1] = t0;
} else {
t0 = $[1];
}
const callback = t0;
let t1;
if ($[2] !== callback) {
t1 = () => {
callback();
};
$[2] = callback;
$[3] = t1;
} else {
t1 = $[3];
}
let t2;
if ($[4] === Symbol.for("react.memo_cache_sentinel")) {
t2 = [];
$[4] = t2;
} else {
t2 = $[4];
}
useEffect(t1, t2);
let t3;
if ($[5] !== setState) {
t3 = [setState];
$[5] = setState;
$[6] = t3;
} else {
t3 = $[6];
}
let t4;
if ($[7] !== t3 || $[8] !== callback) {
t4 = <ValidateMemoization inputs={t3} output={callback} />;
$[7] = t3;
$[8] = callback;
$[9] = t4;
} else {
t4 = $[9];
}
let t5;
if ($[10] !== t4 || $[11] !== state) {
t5 = (
<>
{t4}
{state}
</>
);
$[10] = t4;
$[11] = state;
$[12] = t5;
} else {
t5 = $[12];
}
return t5;
}

export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{}],
};

```
### Eval output
(kind: exception) Output identity changed but inputs did not
logs: ['hello','hello','goodbye','hello','hello','goodbye']
Loading

0 comments on commit c93d4b3

Please sign in to comment.