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

Added invariant for null/undefined create in useEffect, useLayoutEffect #15197

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@JoshuaKGoldberg
Copy link

commented Mar 22, 2019

I considered putting this somewhere closer to where create is used, but also wanted the complaint to be specific to the function & parameter name...

Fixes #15194.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Mar 22, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

okie dokie, following up now...

image

@facebook-github-bot

This comment has been minimized.

Copy link

commented Mar 22, 2019

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sizebot

This comment has been minimized.

Copy link

commented Mar 22, 2019

Details of bundled changes.

Comparing: 5c2b2c0...6c0fb09

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.3% +0.2% 101.38 KB 101.67 KB 26.3 KB 26.35 KB UMD_DEV
react.development.js +0.5% +0.3% 63.92 KB 64.22 KB 17.2 KB 17.25 KB NODE_DEV
React-dev.js +0.6% +0.4% 62.31 KB 62.69 KB 16.53 KB 16.59 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

We try to avoid DEV-only invariants because they can lead to inconsistent behavior between dev and prod. Ideally we would throw right before it would throw anyway. In that case maybe it’s accepable.

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Author

commented Mar 23, 2019

@gaearon I'm not super sure if there's a change you wanted, but I'm interpreting that to mean that this should remove the if (__DEV__)? Applied in the latest commit 😄

@acdlite

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Could you make it a dev-only warning, instead of an invariant? The idea is to avoid an extra check in production, and to make sure the behavior is consistent across dev and prod.

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Author

commented Mar 25, 2019

@acdlite sure - how would you like me to do that? I'm not familiar with the terminology here; does a dev-only warning mean using the warning from shared/warming?

JoshuaKGoldberg added some commits Mar 25, 2019

@alexanderkjeldaas

This comment has been minimized.

Copy link

commented Apr 10, 2019

#15369 is somewhat related to more error checking, but for the return value.

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.