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] Expect component props annotations to be potential objects #29866

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

mvitousek
Copy link
Contributor

@mvitousek mvitousek commented Jun 11, 2024

Stack from ghstack (oldest at bottom):

Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. number as a parameter.

Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.

[ghstack-poisoned]
Copy link

vercel bot commented Jun 11, 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 Jun 11, 2024 8:49pm

mvitousek added a commit that referenced this pull request Jun 11, 2024
Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.

ghstack-source-id: 03622a68d62bb7a4029dd8cf228e62b290b6dd7b
Pull Request resolved: #29866
@react-sizebot
Copy link

react-sizebot commented Jun 11, 2024

Comparing: 270229f...2cd0122

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.80 kB 497.80 kB = 89.24 kB 89.24 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.62 kB 502.62 kB = 89.94 kB 89.94 kB
facebook-www/ReactDOM-prod.classic.js = 597.57 kB 597.57 kB = 105.37 kB 105.37 kB
facebook-www/ReactDOM-prod.modern.js = 571.50 kB 571.50 kB = 101.27 kB 101.27 kB
test_utils/ReactAllWarnings.js Deleted 63.88 kB 0.00 kB Deleted 15.96 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 63.88 kB 0.00 kB Deleted 15.96 kB 0.00 kB

Generated by 🚫 dangerJS against 2cd0122

} else if (params.length === 2) {
// check if second param might be a ref
if (params[1].isIdentifier()) {
const { name } = params[1].node;
return name.includes("ref") || name.includes("Ref");
return (
isValidPropsAnnotation(params[0].node.typeAnnotation) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

i can imagine wanting something different here over time, but given the current logic of the method it makes sense to share. maybe rename to clarify? isValidPropsOrRefAnnotation()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The annotation check is still applying just to the props in both cases, actually--I've refactored the logic here to make that clearer.

…ial objects"

Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.

[ghstack-poisoned]
mvitousek added a commit that referenced this pull request Jun 11, 2024
Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.

ghstack-source-id: cdd0af460c4727fcfb2fafb7c6479a4427ba5b7d
Pull Request resolved: #29866
…ial objects"

Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.

[ghstack-poisoned]
mvitousek added a commit that referenced this pull request Jun 11, 2024
Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.

ghstack-source-id: e2c23348265b7ef651232b962ed7be7f6fed1930
Pull Request resolved: #29866
@mvitousek mvitousek merged commit 2cd0122 into gh/mvitousek/2/base Jun 11, 2024
44 of 50 checks passed
mvitousek added a commit that referenced this pull request Jun 11, 2024
Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.

ghstack-source-id: e2c23348265b7ef651232b962ed7be7f6fed1930
Pull Request resolved: #29866
@mvitousek mvitousek deleted the gh/mvitousek/2/head branch June 11, 2024 21:08
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.

4 participants