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

Enhance dev warnings for forwardRef render function (#13627) #13636

Merged
merged 3 commits into from Sep 13, 2018

Conversation

@andresroberto
Copy link
Contributor

@andresroberto andresroberto commented Sep 13, 2018

  • For 0 parameters: Do not warn because it may be due to usage of the
    arguments object.
  • For 1 parameter: Warn about missing the 'ref' parameter.
  • For 2 parameters: This is the ideal. Do not warn.
  • For more than 2 parameters: Warn about undefined parameters.

Solves #13627

- For 0 parameters: Do not warn because it may be due to usage of the
  arguments object.

- For 1 parameter: Warn about missing the 'ref' parameter.

- For 2 parameters: This is the ideal. Do not warn.

- For more than 2 parameters: Warn about undefined parameters.
@sizebot
Copy link

@sizebot sizebot commented Sep 13, 2018

Details of bundled changes.

Comparing: dde0645...5c832cc

schedule

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
schedule.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
schedule.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

Loading

Copy link
Member

@gaearon gaearon left a comment

Small nits

Loading

return null;
}
it('should not warn if the render function provided does not use any parameter', () => {
const arityOfZero = () => null;
Copy link
Member

@gaearon gaearon Sep 13, 2018

Choose a reason for hiding this comment

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

Can we make this more realistic? Such as a real pass through wrapper. Otherwise intent is not clear.

Loading

'forwardRef render functions accept exactly two parameters: props and ref. %s',
render.length === 1
? 'Did you forget to use the ref parameter?'
: 'Any additional parameter will be undefined',
Copy link
Member

@gaearon gaearon Sep 13, 2018

Choose a reason for hiding this comment

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

Please add period to the sentence.

Loading

@andresroberto andresroberto force-pushed the forwardref-parameters-warning branch 3 times, most recently from e2d493d to ae395ba Sep 13, 2018
@andresroberto andresroberto force-pushed the forwardref-parameters-warning branch from 29922f4 to 5c832cc Sep 13, 2018
@gaearon gaearon merged commit ecbf7af into facebook:master Sep 13, 2018
1 check passed
Loading
@gaearon
Copy link
Member

@gaearon gaearon commented Sep 13, 2018

Thanks

Loading

@andresroberto
Copy link
Contributor Author

@andresroberto andresroberto commented Sep 13, 2018

Thank you! I am happy to contribute 😄

Loading

@gaearon gaearon mentioned this pull request Sep 13, 2018
Simek added a commit to Simek/react that referenced this issue Oct 25, 2018
…acebook#13636)

* Enhance dev warnings for forwardRef render function

- For 0 parameters: Do not warn because it may be due to usage of the
  arguments object.

- For 1 parameter: Warn about missing the 'ref' parameter.

- For 2 parameters: This is the ideal. Do not warn.

- For more than 2 parameters: Warn about undefined parameters.

* Make test cases for forwardRef warnings more realistic

* Add period to warning sentence
jetoneza added a commit to jetoneza/react that referenced this issue Jan 23, 2019
…acebook#13636)

* Enhance dev warnings for forwardRef render function

- For 0 parameters: Do not warn because it may be due to usage of the
  arguments object.

- For 1 parameter: Warn about missing the 'ref' parameter.

- For 2 parameters: This is the ideal. Do not warn.

- For more than 2 parameters: Warn about undefined parameters.

* Make test cases for forwardRef warnings more realistic

* Add period to warning sentence
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants