-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Comments
Can you provide a minimal project reproducing this? |
I get similar errors here: https://codesandbox.io/s/jvX5PQol That can serve as a good starting point. |
@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 |
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). |
|
cc @acdlite, maybe we need feature test after all? This is very confusing. |
No, I don’t think so. |
@jimbolla in my case I have the store in the provider. It still display's the error. |
@gartz Please provide a minimal project reproducing the issue that isn’t hosted in an online editor. Thank you! |
@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. |
Thanks! |
Looks like it. Here's what I get in a fresh CRA app:
|
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. |
@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. |
The least we could do is add a warning when this happens so the developer has more context. Maybe this would be satisfactory? |
Do I understand right that this wasn’t a problem before only because we didn’t attempt to capture/log the error ourselves? |
Since the error is printed to the console anyway by the browser, I suppose we could just omit Currently the console looks like:
After proposed change, it would look like:
Of course |
The case I'm more worried about is when null is passed to an error boundary. |
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.
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 |
Yeah we should at least do that for |
|
We really should have this thread on the |
Filed facebook/react#10321. |
Well, since this is the rare bug with React itself, I'm closing this out. Thanks @gartz for the original heads up! |
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. |
The following error crashes the app when using React 16 beta.
Other error messages that might be relevant:
By removing react-redux or downgrading to React 15.x it works correctly.
The text was updated successfully, but these errors were encountered: