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

Fixes missing default funcs as props in `BlockEditorProvider` #17036

Merged

Conversation

@getdave
Copy link
Contributor

commented Aug 14, 2019

The component assumes that the onChange and onInput props are always present. However, there are times where this may not be the case such as when the BlockPreview component uses the provider to render the previews. In such cases onChange is called but is undefined which causes an error to be thrown.

To avoid this we provide lodash noop as defaults for both “change” props.

An example of when an error is thrown can be seen in this PR

Automattic/wp-calypso#35333

Questions

  • Should we test for isFunction as well?
  • Is noop a good default?
  • Should I also fix the React Native version? Who do I need to ping about this?

How has this been tested?

Running this branch against Automattic/wp-calypso#35333 causes the error related to onChange to disappear.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
Previously the component assumed that the onChange and onInput props would always be present. However, there are times where this may not be the case such as when the BlockPreview component uses the provider to render the previews. In such cases `onChange` is called but is undefined which causes an error to be thrown.

To avoid this we provide lodash noop as defaults for both “change” props.
@getdave getdave requested a review from youknowriad Aug 14, 2019
@getdave getdave requested review from ellatrix and talldan as code owners Aug 14, 2019
@getdave getdave self-assigned this Aug 14, 2019
@getdave getdave referenced this pull request Aug 14, 2019
3 of 3 tasks complete
@getdave getdave requested a review from retrofox Aug 14, 2019
Copy link
Contributor

left a comment

That makes sense to me but I'm wondering why these get called in previews? This could hint to a hidden bug where we're calling onChange where we shouldn't.

@getdave getdave merged commit d83c239 into master Aug 15, 2019
22 checks passed
22 checks passed
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Filter opened
Details
Milestone It
Details
Milestone It
Details
Milestone It
Details
Milestone It
Details
Milestone It
Details
Milestone It
Details
Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@getdave getdave deleted the fix/block-editor-provider-ensure-default-change-props branch Aug 15, 2019
@senadir senadir added this to the Gutenberg 6.4 milestone Aug 25, 2019
gziolo added a commit that referenced this pull request Aug 29, 2019
Previously the component assumed that the onChange and onInput props would always be present. However, there are times where this may not be the case such as when the BlockPreview component uses the provider to render the previews. In such cases `onChange` is called but is undefined which causes an error to be thrown.

To avoid this we provide lodash noop as defaults for both “change” props.
gziolo added a commit that referenced this pull request Aug 29, 2019
Previously the component assumed that the onChange and onInput props would always be present. However, there are times where this may not be the case such as when the BlockPreview component uses the provider to render the previews. In such cases `onChange` is called but is undefined which causes an error to be thrown.

To avoid this we provide lodash noop as defaults for both “change” props.
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
Previously the component assumed that the onChange and onInput props would always be present. However, there are times where this may not be the case such as when the BlockPreview component uses the provider to render the previews. In such cases `onChange` is called but is undefined which causes an error to be thrown.

To avoid this we provide lodash noop as defaults for both “change” props.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.