[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

Sign up for GitHub

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

Jump to bottom

refactor: replace use-subscription #6246

Closed
wants to merge 2 commits into from
Closed

refactor: replace use-subscription #6246

wants to merge 2 commits into from

Conversation

SukkaW
Copy link
@SukkaW SukkaW commented May 13, 2022 •
edited
Loading

Description

Closes # (NO_ISSUE)

Type of change

  • Documentation
  • Code refactoring (Restructuring existing code w/o changing its observable behavior)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (a fix or feature that would make something no longer possible to do/require old user must upgrade their Mask Network to this new version)

Checklist

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
    • I have removed all in development console.logs
    • I have removed all commented code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have read Internationalization Guide and moved text fields to the i18n JSON file.

If this PR depends on external APIs:

  • I have configured those APIs with CORS headers to let extension requests get passed.
    • chrome extension: chrome-extension://[id]
    • firefox extension: moz-extension://[id]
  • I have delegated all web requests to the background service via the internal RPC bridge.

Back in 2019, React released the first version of use-subscription (facebook/react#15022). At the time, we only have very limited information about concurrent rendering.

In 2020, React provides a first-party official API useMutableSource (reactjs/rfcs#147, facebook/react#18000):

... enables React components to safely and efficiently read from a mutable external source in Concurrent Mode.

React 18 introduces useMutableSource's replacement useSyncExternalStore (see details here: reactwg/react-18#86), and React changes use-subscription implementation to use useSyncExternalStore directly: facebook/react#24289

In React 18, React.useSyncExternalStore is a built-in replacement for useSubscription.

This PR makes useSubscription simply use React.useSyncExternalStore when available. For pre-18, it uses a use-sync-external-store shim which is very similar in use-subscription but fixes some flaws with concurrent rendering.

And according to use-subscription:

You may now migrate to use-sync-external-store directly instead, which has the same API as React.useSyncExternalStore. The use-subscription package is now a thin wrapper over use-sync-external-store and will not be updated further.

The PR does exactly that:

  • Removes use-subscription
  • Adds the use-sync-external-store to the dependencies.
    • The use-sync-external-store package enables compatibility with React 16 and React 17.
  • Replace useSubscription usage with useSyncExternalStoreWithSelector
    • Unlike useSyncExternalStore shim, useSyncExternalStoreWithSelector provides the built-in memo support to prevent infinite loop rendering.

Jack-Works
Jack-Works requested changes May 16, 2022
View reviewed changes
Copy link
Member
@Jack-Works Jack-Works left a comment

Choose a reason for hiding this comment

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

Thanks for your interest! I have some review suggestions

packages/shared-base/src/utils/subscription.ts
/**
* (Synchronously) returns the current value of our subscription.
*/
getCurrentValue: () => T
Copy link
Member
@Jack-Works Jack-Works May 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think you should add a getServerValue?(): T to Subscription type, and export the following hooks from the shared (React hooks should not inside the shared-base) package:

function useSubscription<T>(sub: Subscription<T>): T {
    return useSyncExternalStoreWithSelector(
        sub.subscribe,
        sub.getCurrentValue,
        sub.getServerValue || sub.getCurrentValue,
        (s) => s,
    )
}

It will make the migration easier.

Copy link
Author
@SukkaW SukkaW May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The approach is only good for migration, not for future refactoring.

Consider this: What happened if different logic for getServerValue would be applied across different useSyncExternalStoreWithSelector usage in the future? What happened if multiple subscriptions will be merged into one store and then fully utilize the selector feature in the future?

IMHO, we should keep useSyncExternalStoreWithSelector as it is.

Copy link
Member

Choose a reason for hiding this comment

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

We haven't made a plan to use a full-featured store like zustand or redux, thus I think easy migration is more important.

What happened if different logic for getServerValue would be applied across different useSyncExternalStoreWithSelector usage in the future?

Can you explain this part?

@SukkaW SukkaW closed this Jun 17, 2022
@SukkaW SukkaW deleted the replace-use-subscription branch June 17, 2022 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jack-Works Jack-Works Jack-Works requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SukkaW @Jack-Works