Skip to content
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

[compiler] Infer deps configuration #31616

Merged
merged 5 commits into from
Nov 22, 2024
Merged

[compiler] Infer deps configuration #31616

merged 5 commits into from
Nov 22, 2024

Conversation

jbrown215
Copy link
Contributor

Adds a way to configure how we insert deps for experimental purposes.

[
  {
    module: 'react',
    imported: 'useEffect',
    numRequiredArgs: 1,
  },
  {
    module: 'MyExperimentalEffectHooks',
    imported: 'useExperimentalEffect',
    numRequiredArgs: 2,
  },
]

would insert dependencies for calls of useEffect imported from react if they have 1 argument and calls of useExperimentalEffectfromMyExperimentalEffectHooks` if they have 2 arguments. The pushed dep array is appended to the arg list.

Copy link

vercel bot commented Nov 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 22, 2024 8:10pm

Comment on lines 269 to 278
inferEffectDependencies: z
.nullable(
z.array(
z.object({
module: z.string(),
imported: z.string(),
numRequiredArgs: z.number(),
}),
),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit (feel free to ignore), it would be nice to make this consistent with other external function config options by using the ExternalFunctionSchema type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh nice!

Comment on lines 186 to 188
module: 'react',
imported: 'useSpecialEffect',
numRequiredArgs: 2,
Copy link
Contributor

@mofeiZ mofeiZ Nov 22, 2024

Choose a reason for hiding this comment

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

Nit: could we change this import to be not from react? It would be great to be able to evaluate all fixture tests, and adding an unknown import from react might mean monkey patching react

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of test-fixture specific defaults, could we also add a default value to testComplexConfigDefaults so that playground users can use @inferEffectDependencies?

() => new Map<string, number>(),
);
moduleTargets.set(effectTarget.imported, effectTarget.numRequiredArgs);
autodepFnConfigs.set(effectTarget.module, moduleTargets);
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit: is this line needed (since we already have getOrInsertWith`)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no it is not :)

Copy link
Contributor

@mofeiZ mofeiZ left a comment

Choose a reason for hiding this comment

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

Nice, let's go!!

@mofeiZ mofeiZ merged commit 2a9f4c0 into facebook:main Nov 22, 2024
18 checks passed
github-actions bot pushed a commit to code/lib-react that referenced this pull request Nov 23, 2024
Adds a way to configure how we insert deps for experimental purposes.

```
[
  {
    module: 'react',
    imported: 'useEffect',
    numRequiredArgs: 1,
  },
  {
    module: 'MyExperimentalEffectHooks',
    imported: 'useExperimentalEffect',
    numRequiredArgs: 2,
  },
]
```

would insert dependencies for calls of `useEffect` imported from `react`
if they have 1 argument and calls of useExperimentalEffect` from
`MyExperimentalEffectHooks` if they have 2 arguments. The pushed dep
array is appended to the arg list.

DiffTrain build for [2a9f4c0](facebook@2a9f4c0)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Nov 23, 2024
Adds a way to configure how we insert deps for experimental purposes.

```
[
  {
    module: 'react',
    imported: 'useEffect',
    numRequiredArgs: 1,
  },
  {
    module: 'MyExperimentalEffectHooks',
    imported: 'useExperimentalEffect',
    numRequiredArgs: 2,
  },
]
```

would insert dependencies for calls of `useEffect` imported from `react`
if they have 1 argument and calls of useExperimentalEffect` from
`MyExperimentalEffectHooks` if they have 2 arguments. The pushed dep
array is appended to the arg list.

DiffTrain build for [2a9f4c0](facebook@2a9f4c0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants