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

fix: implementation of useSyncExternalStore #291

Merged
merged 3 commits into from
Jan 11, 2023

Conversation

exuanbo
Copy link
Contributor

@exuanbo exuanbo commented Jan 10, 2023

Please describe the changes this PR makes and why it should be merged:

Status

  • Code changes have been tested against prettier, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the codebase
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
    • This PR changes the internal workings with no modifications to the external API (bug fixes, performance improvements)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

This PR updates the hook implementation used by preact.

Refs: preactjs/preact#3663 and preactjs/preact@528a776

Comment on lines 247 to 254
// This implementation is grokked straight from Preact (ref: https://github.com/preactjs/preact/blob/528a7760c1cdec7515d514c71875860528e6dc12/compat/src/index.js#L142-L171)
// (c) 2015-present Jason Miller
export const useSyncExternalStore = (subscribe, getSnapshot) => {
const [state, setState] = useState(getSnapshot);

const value = getSnapshot();

useLayoutEffect(() => {
if (value !== state) {
setState(() => value);
}
}, [subscribe, value, getSnapshot]);
const [{ instance }, forceUpdate] = useState({
instance: { value, getSnapshot }
});
Copy link
Contributor

@SukkaW SukkaW Jan 10, 2023

Choose a reason for hiding this comment

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

It looks like preact's new useSyncExternalStore is now ported from react's use-sync-external-store/shim. So you should do it too.

Please do copy their comments describing how it breaks rule of hooks. Million.js doesn't have concurrent rendering (Both preact and Million.js performs synchronous rendering just like React pre-18), and it is the only reason why react's shim also works for Million.js (and preact).

https://github.com/facebook/react/blob/main/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js

@SukkaW
Copy link
Contributor

SukkaW commented Jan 10, 2023

Thanks for your contribution! I have just approved the CI.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3885856542

  • 9 of 45 (20.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.3%) to 59.311%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/react/hooks.ts 9 45 20.0%
Files with Coverage Reduction New Missed Lines %
packages/react/hooks.ts 2 28.17%
Totals Coverage Status
Change from base Build 3798731700: -0.3%
Covered Lines: 1415
Relevant Lines: 2484

💛 - Coveralls

@SukkaW SukkaW merged commit af0c1fd into aidenybai:main Jan 11, 2023
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

Successfully merging this pull request may close these issues.

3 participants