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

Incompatible with React 16 beta #756

Closed
gartz opened this issue Jul 28, 2017 · 25 comments
Closed

Incompatible with React 16 beta #756

gartz opened this issue Jul 28, 2017 · 25 comments

Comments

@gartz
Copy link

gartz commented Jul 28, 2017

The following error crashes the app when using React 16 beta.

React caught an error thrown by Provider. You should fix this error in your code. Consider adding an error boundary to your tree to customize error handling behavior. See https://fb.me/react-error-boundaries for more information.

Error: null

The error is located at: 
    in Provider

The error was thrown at:

Other error messages that might be relevant:

Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in.

invariant.js?c115:44 Uncaught Error: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in.

By removing react-redux or downgrading to React 15.x it works correctly.

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2017

Can you provide a minimal project reproducing this?

@timdorr
Copy link
Member

timdorr commented Jul 28, 2017

I get similar errors here: https://codesandbox.io/s/jvX5PQol That can serve as a good starting point.

@jimbolla
Copy link
Contributor

@timdorr Your errors go away if you give Provider or App a store: https://codesandbox.io/s/nZXwy0l57

It does seem though in the case where connect doesn't find a store, that the console does get a lot of noise. I wonder if react-redux could provide a better DX when targeting react 16.

@timdorr
Copy link
Member

timdorr commented Jul 28, 2017

Should Provider or connect provide an error boundary? I haven't read too much into cDC, but I assume we can re-throw errors that we don't want to handle (e.g., errors from within the wrapped component).

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2017

Error: null looks odd. I wonder if this is because it’s in CodeSandbox. Can you reproduce the same error locally?

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2017

cc @acdlite, maybe we need feature test after all? This is very confusing.

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2017

Should Provider or connect provide an error boundary?

No, I don’t think so.

@gartz
Copy link
Author

gartz commented Jul 28, 2017

@jimbolla in my case I have the store in the provider. It still display's the error.

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2017

@gartz Please provide a minimal project reproducing the issue that isn’t hosted in an online editor. Thank you!

@gartz
Copy link
Author

gartz commented Jul 28, 2017

@gaearon I will work on that by the end of the day, I will need to remove some proprietary stuff from the project I'm using to test this and try to isolate the error.

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2017

Thanks!

@timdorr
Copy link
Member

timdorr commented Jul 28, 2017

Error: null looks odd. I wonder if this is because it’s in CodeSandbox. Can you reproduce the same error locally?

Looks like it. Here's what I get in a fresh CRA app:

React caught an error thrown by Connect(BaseApp). You should fix this error in your code. Consider adding an error boundary to your tree to customize error handling behavior. See https://fb.me/react-error-boundaries for more information.

Invariant Violation: Could not find "store" in either the context or props of "Connect(BaseApp)". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(BaseApp)".

The error is located at: 
    in Connect(BaseApp) (at index.js:15)
    in Provider (at index.js:14)

@timdorr
Copy link
Member

timdorr commented Jul 28, 2017

I'm guessing because CodeSandbox runs in an iframe and is shipped code/data externally, it doesn't have the right security context to access the error object or stack trace.

@sebmarkbage
Copy link

sebmarkbage commented Jul 28, 2017

@gaearon FWIW the feature test wouldn't cover this case. AFAIK we don't have a good way to detect that some part of the code is from a different origin than another. Either Chrome fixes https://bugs.chromium.org/p/chromium/issues/detail?id=701371 or we can force use of Break On Caught Exceptions for debugging which is not great neither.

@acdlite
Copy link
Contributor

acdlite commented Jul 28, 2017

The least we could do is add a warning when this happens so the developer has more context. Maybe this would be satisfactory?

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2017

Do I understand right that this wasn’t a problem before only because we didn’t attempt to capture/log the error ourselves?

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2017

Since the error is printed to the console anyway by the browser, I suppose we could just omit Error: null when it is null (since that isn’t very helpful anyway). Then it would just appear as an error + a separate message with a component stack. Similarly, we should omit the JS stack if we don’t have it.

Currently the console looks like:

Uncaught Error: Could not find "store" in either the context or props of "Connect(BaseApp)". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(BaseApp)".
    at invariant (dll.js:12)
    at new Connect (dll.js:12)
    at constructClassInstance (dll.js:26)
    at updateClassComponent (dll.js:26)
    at beginWork (dll.js:26)
    at performUnitOfWork (dll.js:26)
    at workLoop (dll.js:26)
    at HTMLUnknownElement.callCallback (dll.js:26)
    at Object.invokeGuardedCallback (dll.js:26)
    at invokeGuardedCallback (dll.js:26)



React caught an error thrown by Connect(BaseApp). You should fix this error in your code. Consider adding an error boundary to your tree to customize error handling behavior. See https://fb.me/react-error-boundaries for more information.

Error: null

The error is located at: 
    in Connect(BaseApp)
    in Provider

The error was thrown at:

After proposed change, it would look like:

Uncaught Error: Could not find "store" in either the context or props of "Connect(BaseApp)". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(BaseApp)".
    at invariant (dll.js:12)
    at new Connect (dll.js:12)
    at constructClassInstance (dll.js:26)
    at updateClassComponent (dll.js:26)
    at beginWork (dll.js:26)
    at performUnitOfWork (dll.js:26)
    at workLoop (dll.js:26)
    at HTMLUnknownElement.callCallback (dll.js:26)
    at Object.invokeGuardedCallback (dll.js:26)
    at invokeGuardedCallback (dll.js:26)



React caught an error thrown by Connect(BaseApp). You should fix this error in your code. Consider adding an error boundary to your tree to customize error handling behavior. See https://fb.me/react-error-boundaries for more information.

The error is located at: 
    in Connect(BaseApp)
    in Provider

Of course null could get legitimately thrown. But then printing Error: null is not giving that much more information anyway.

@acdlite
Copy link
Contributor

acdlite commented Jul 29, 2017

The case I'm more worried about is when null is passed to an error boundary.

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2017

The least we could do is add a warning when this happens so the developer has more context.

Depends on the wording, but I don’t think printing something like “React could not get the error message because the script is served from a different domain.” or something like this would be very useful. Since if the user is in a certain environment (e.g. jsfiddle) they probably can’t influence it anyway, and it’s just annoying to see unactionable warnings for a technical issue that seems like library’s concern.

The case I'm more worried about is when null is passed to an error boundary.

I figured we can provide a fake / our own error object in this case, what do you think? I was thinking maybe we should do this for all primitives. Because they don’t carry valuable info, and because it’s too easy to do if (this.state.error) check without realizing it might be falsy.

@acdlite
Copy link
Contributor

acdlite commented Jul 29, 2017

Yeah we should at least do that for null, not sure about other non-error values.

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2017

throw '', throw false, and throw undefined also don’t sound like fun.

@sebmarkbage
Copy link

We really should have this thread on the react repo.

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2017

Filed facebook/react#10321.

@timdorr
Copy link
Member

timdorr commented Jul 29, 2017

Well, since this is the rare bug with React itself, I'm closing this out. Thanks @gartz for the original heads up!

@timdorr timdorr closed this as completed Jul 29, 2017
@mnicole
Copy link

mnicole commented Oct 17, 2017

in case it helps anyone in the future, i just upgraded to react 16 and was using react-redux, react-router and react-router-redux and getting the same error. i had to use the @next version of react-router-redux to resolve it.

@reduxjs reduxjs deleted a comment from HuangHongRui Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants