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

Bug: unstable_useMutableSource throws error when mutated before subscribe #19948

Closed
SreeniIO opened this issue Oct 2, 2020 · 3 comments · Fixed by #20665
Closed

Bug: unstable_useMutableSource throws error when mutated before subscribe #19948

SreeniIO opened this issue Oct 2, 2020 · 3 comments · Fixed by #20665

Comments

@SreeniIO
Copy link

SreeniIO commented Oct 2, 2020

React version: 0.0.0-experimental-94c0244ba

Steps To Reproduce

Mutating the external source during the initial render before subscribe throws error.

https://codesandbox.io/s/usemutablesource-7nbs6?file=/src/App.js

image

  1. Navigate to the above code sandbox
  2. Click on the Mutate button. Final Value is rendered without any error. (Success Mode)
  3. Toggle to Error Mode and click on the Mutate button again.
  4. Error thrown (and the UI is rendered with the Final Value when run from vscode)

Note: In our actual codebase (private), we are seeing this error at different instances e.g. component getting updated due to setState call in useLayoutEffect etc.

The current behavior

Seeing errors in development build.

image

The expected behavior

These errors thrown by useMutableSource should not showup on the UI and not passed to the error boundary as the component is eventually re-rendered with the latest state without any error.

@SreeniIO SreeniIO added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Oct 2, 2020
@bvaughn bvaughn self-assigned this Oct 2, 2020
@SreeniIO
Copy link
Author

SreeniIO commented Oct 7, 2020

I'm using this workaround for now

  try {
    value = useMutableSource(mutableSource, getSnapshot, defaultSubscribe);
  } catch (error) {
    throw new Promise((resolve) => resolve());
  }

https://codesandbox.io/s/usemutablesource-forked-0suof?file=/src/App.js:1126-1269

@bvaughn
Copy link
Contributor

bvaughn commented Jan 26, 2021

Proposed solution (really just a better warning and error message) in #20665

@bvaughn
Copy link
Contributor

bvaughn commented Jan 29, 2021

@SreeniIO Thanks for the repro case and discussion. I've landed a solution in #20665 and I'd suggest reconsidering the workaround you mentioned. My reasoning is this (copied from the PR description):

Mutating a mutable source (or any external variable) during render is not expected to be supported. However this error message misattributes the mutation as a React bug.

Should this use case be supported?

This seems very similar lazily initializing a ref so it might be tempting to think it's okay. Mutable sources are external though, so modifying them during render is a side effect which makes it unsafe and unsupported. So while we could "support" this case in a very limited way, we shouldn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants