Skip to content

Conversation

@jbrown215
Copy link
Contributor

@jbrown215 jbrown215 commented Jan 22, 2025

Summary: Summary: Correctly supports React.useEffect when React is imported as import * as React from 'react'
(as well as other namespaces as specified in the config).

Test Plan:

Stack created with Sapling. Best reviewed with ReviewStack.

} else if (
value.kind === 'PropertyLoad' &&
typeof value.property === 'string' &&
autodepModuleLoads.has(value.object.identifier.id)
Copy link
Contributor

@mofeiZ mofeiZ Mar 3, 2025

Choose a reason for hiding this comment

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

codestyle nit (feel free to take or leave it) -- I find it's easier to read long nested if-else chains if the outer / inner ifs are simple. e.g.

for (const instr of block.instructions) {
  if (value.kind === "FunctionExpression") {
    ...
  } else if (value.kind === "PropertyLoad") {
    const moduleTarget = autodepModuleLoads.get(
      value.object.identifier.id,
    );
    if (...) {
      ...
    }
  } else if (value.kind === "CallExpression" || value.kind === "MethodCall") {
    ...
  } 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's reasonable! I see why that would be easier to read

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.

Awesome, shipit! 🛳️

@@ -1,9 +1,6 @@
// @inferEffectDependencies
import * as React from 'react';
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh makes sense, thanks for checking the ImportDefault case!

Summary: Summary: Correctly supports React.useEffect when React is imported as `import * as React from 'react'`
(as well as other namespaces as specified in the config).

Test Plan:
@jbrown215 jbrown215 merged commit bdce84a into facebook:main Mar 3, 2025
23 checks passed
@jbrown215 jbrown215 deleted the pr32162 branch March 3, 2025 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants