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

Update useControlledState for StrictMode #1375

Closed
wants to merge 7 commits into from

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Dec 15, 2020

The current useControlledState has side effects, this causes an issue StrictMode
See initial work here https://github.com/adobe/react-spectrum/compare/tests-strict-mode#diff-927eebaa6f590e895634eae7f762db38e91f5f30a71fb8960c620cabfa59d058L50

Closes #1375, #1440, #1611

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

The current useControlledState has side effects, this causes an issue StrictMode
@adobe-bot
Copy link

Build successful! 🎉

};
setStateValue(updateFunction);
let oldValue = stateRef.current;
let interceptedValue = value(oldValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line doesn't seem like it is the same. I'm having trouble figuring out what the expected different between oldValue and stateRef.current in the original code would be and what they are virtually the same in the current code.

In addition, the comment for this section of code seems out of date.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, yep, that comment is now out of date
and yes, it's not quite the same as before, which is why we've pulled it out into its own PR, the issue was that the old way of doing it had a side effect
it used to update the state, once that update was complete, it would call onChange
now it queues the state update and calls onChange, possibly before the state has been committed

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested that this fixed a strict mode issue (added a link) and updated flow looks fine to me.

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@snowystinger
Copy link
Member Author

An update, so far neither this nor the original behavior is quite correct. We're still looking into it.
Here's a codesandbox with some tests that show how both implementations fall short in different ways
https://codesandbox.io/s/jolly-cache-8esil?file=/src/App.js

@adobe-bot
Copy link

Build successful! 🎉

@snowystinger
Copy link
Member Author

Closing because we've determined we cannot implement this feature and the current implementation has a bug, we'll be deprecating the set state callback functionality, see #2320 for details along with its PR to deprecate which shows how we'll work around this issue for ourselves

@snowystinger snowystinger deleted the update-useControlledState branch September 21, 2021 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants