Skip to content

Commit

Permalink
named importing hooks
Browse files Browse the repository at this point in the history
  • Loading branch information
@dai-shi
dai-shi committed Feb 16, 2020
1 parent b88b623 commit 26c8a16
Showing 1 changed file with 18 additions and 11 deletions.
29 changes: 18 additions & 11 deletions src/index.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
/* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/ban-ts-ignore */

import React from 'react';
import React, {
createElement,
createContext as createContextOrig,
useCallback,
useContext as useContextOrig,
useMemo,
useRef,
} from 'react';

const createMutableSource = (React as any).createMutableSource as any;
const useMutableSource = (React as any).useMutableSource as any;
const createMutableSource = 'NOT_AVAILABLE_YET' as any;
const useMutableSource = 'NOT_AVAILABLE_YET' as any;

const SOURCE_SYMBOL = Symbol();

Expand All @@ -12,16 +19,16 @@ type ContextValue<Value> = {
[SOURCE_SYMBOL]: any;
};

const createProvider = <Value>(OrigProvider: React.Provider<ContextValue<Value>>) => {
const createProvider = <Value>(ProviderOrig: React.Provider<ContextValue<Value>>) => {
const Provider: React.FC<{ value: Value }> = ({ value, children }) => {
const ref = React.useRef({ value, listeners: new Set<() => void>() });
const ref = useRef({ value, listeners: new Set<() => void>() });
ref.current.value = value;
ref.current.listeners.forEach((listener) => listener());
const contextValue = React.useMemo(() => {
const contextValue = useMemo(() => {
const source = createMutableSource(ref, () => ref.current.value);
return { [SOURCE_SYMBOL]: source };
}, []);
return React.createElement(OrigProvider, { value: contextValue }, children);
return createElement(ProviderOrig, { value: contextValue }, children);
};
return React.memo(Provider);
};
Expand All @@ -40,7 +47,7 @@ export const createContext = <Value>(defaultValue: Value) => {
const source = createMutableSource({ current: defaultValue }, {
getVersion: () => defaultValue,
});
const context = React.createContext(
const context = createContextOrig(
{ [SOURCE_SYMBOL]: source },
) as unknown as React.Context<Value>; // HACK typing
context.Provider = createProvider(
Expand Down Expand Up @@ -68,16 +75,16 @@ export const useContextSelector = <Value, Selected>(
context: React.Context<Value>,
selector: (value: Value) => Selected,
) => {
const { [SOURCE_SYMBOL]: source } = React.useContext(
const { [SOURCE_SYMBOL]: source } = useContextOrig(
context,
) as unknown as ContextValue<Value>; // HACK typing
if (!source) {
throw new Error('useContextSelector requires special context');
}
const getSnapshot = React.useCallback((
const getSnapshot = useCallback((
ref: React.MutableRefObject<{ value: Value }>,
) => selector(ref.current.value), [selector]);
const subscribe = React.useCallback((
const subscribe = useCallback((
ref: React.MutableRefObject<{ value: Value; listeners: Set<() => void> }>,
callback: () => void,
) => {
Expand Down

8 comments on commit 26c8a16

@JeremyRH
Copy link

Choose a reason for hiding this comment

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

const listener = () => {
  const nextSelected = selector(ref.current.value);
  if (!Object.is(selected, nextSelected)) {
    callback();
    selected = nextSelected;
  }
};

selector(ref.current.value) can read from stale props if selector is: state => state[props.someId].
This is the problem with selectors using props. It is not safe to read from props if they will be eagerly evaluated and they have to be eagerly evaluated to see if the derived value has changed.

@dai-shi
Copy link
Owner Author

Choose a reason for hiding this comment

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

@JeremyRH Thanks for the comment. I understand that's one of the biggest question.

What I assume is:

  • if the selector is changed (possibly with props change), useMutableSource will unsubscribe and re-subscribe in somewhat a sync manner.
  • even if the callback is called before that, useMutableSource would detect updates correctly with getVersion, and by the time it calls getSnapshot, it will use the updated selector. So, the worst case could be an extra re-render with same (but can be ref unequal) snapshot.

So, there should be no stale props issue like react-redux v7.1. The remaining concern is performance because it re-subscribes every time props change, or every time it renders if the selector isn't wrapped by useCallback.

@JeremyRH
Copy link

Choose a reason for hiding this comment

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

You can use a ref for the selector to avoid re-subscribes:

const selectorRef = useRef();
if (selectorRef.current !== selector) {
  selectorRef.current = selector;
}

const subscribe = useCallback((ref, callback) => {
  let selected = selectorRef.current(ref.current.value);
  const listener = () => {
    const nextSelected = selectorRef.current(ref.current.value);
    if (!Object.is(selected, nextSelected)) {
      callback();
      selected = nextSelected;
    }
  };
  const { listeners } = ref.current;
  listeners.add(listener);
  return () => listeners.delete(callback);
}, [selectorRef]); // selectorRef never changes, only .current property.

@dai-shi
Copy link
Owner Author

Choose a reason for hiding this comment

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

@JeremyRH
It absolutely works in legacy mode. But from my experience in reduxjs/react-redux#1509, refs won't work well in concurrent mode.
One of the issues in your snippet is selectorRef.current = selector;. In CM, after this line, React will throw away the result without committing, which ends up keeping an improper selector in the ref.

@JeremyRH
Copy link

Choose a reason for hiding this comment

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

You are calling the selector outside of the render phase (inside listener) and it is only used to schedule a new render. I don’t think it matters if selectorRef.current is an improper selector. Maybe if the improper selector prevented a new render it would be an issue but I don’t know how that can happen.

getSnapshot would not use selectorRef.current so it would always use a proper selector.

@dai-shi
Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, the improper selector can prevent triggering re-render, which is an issue. That can happen.

You are probably right about getSnapshot. I wasn't aware that. So, we might not even need useCallback?

@JeremyRH
Copy link

Choose a reason for hiding this comment

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

Yes, the improper selector can prevent triggering re-render, which is an issue. That can happen.

Can you show an example? When React renders but does not commit, doesn't React automatically schedule a new render or commit for later?

So, we might not even need useCallback?

You need it for subscribe or else it will resubscribe every render but new getSnapshot functions do not cause resubscribes.

@dai-shi
Copy link
Owner Author

Choose a reason for hiding this comment

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

Can you show an example?

It's really hard to show it in an easy way. You could see the thrown-away behavior in https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode. It won't show all the counts.

When React renders but does not commit, doesn't React automatically schedule a new render or commit for later?

It will usually schedule a new render (without commit).

new getSnapshot functions do not cause resubscribes.

But, it fails to re-use the previous snapshot. Found this description in the RFC:

  // Because the snapshot depends on props, it has to be created inline.
  // useCallback() memoizes the function though,
  // which lets useMutableSource() know when it's safe to reuse a snapshot value.

Please sign in to comment.