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

feat: new hook useGetSet #530

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/__docs__/migrating-from-react-use.story.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ Not implemented yet

#### useGetSet

Not implemented yet
See [useGetSet](/docs/state-useGetSet--example)

#### useGetSetState

Expand Down
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export { useMap } from './useMap/useMap';
export { useMediatedState } from './useMediatedState/useMediatedState';
export { usePrevious } from './usePrevious/usePrevious';
export { useSafeState } from './useSafeState/useSafeState';
export { useGetSet } from './useGetSet/useGetSet';
export { useSet } from './useSet/useSet';
export { useToggle } from './useToggle/useToggle';
export { useThrottledState } from './useThrottledState/useThrottledState';
Expand Down
12 changes: 12 additions & 0 deletions src/useGetSet/__docs__/example.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as React from 'react';
import { useGetSet } from '../..';

export const Example: React.FC = () => {
const [get, set] = useGetSet(0);
const onClick = () => {
setTimeout(() => {
set(get() + 1);
}, 1000);
};
return <button onClick={onClick}>Clicked: {get()}</button>;
};
38 changes: 38 additions & 0 deletions src/useGetSet/__docs__/story.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { Canvas, Meta, Story } from '@storybook/addon-docs/blocks';
import { Example } from './example.stories';
import { ImportPath } from '../../storybookUtil/ImportPath';

<Meta title="State/useGetSet" component={Example} />

# useGetSet

React state hook that returns state getter function instead of raw state itself, this prevents subtle bugs when state is used in nested functions.

#### Example

<Canvas>
<Story story={Example} inline />
</Canvas>

## Reference

```ts
export function useGetSet<S>(
initialState: IInitialState<S>
): [get: () => S, set: (nextState: INextState<S>) => void];
```

#### Importing

<ImportPath />

#### Arguments

- _**initialState**_ _`IInitialState<S>`_ - initial state or initial state setter as for `useState`

#### Return

Returns array alike `useState` does, but instead return a state value it return a getter function.

- _**[0]**_ _`S`_ - getter of current state
- _**[1]**_ _`(nextState?: INewState<S>) => void`_ - state setter as for `useState`.
62 changes: 62 additions & 0 deletions src/useGetSet/__tests__/dom.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { renderHook, act } from '@testing-library/react-hooks/dom';
import { useGetSet } from '../..';

const setUp = (initialValue: any) => renderHook(() => useGetSet(initialValue));

beforeEach(() => {
jest.useFakeTimers();
});

describe('useGetSet', () => {
it('should be defined', () => {
expect(useGetSet).toBeDefined();
});

it('should render', () => {
const { result } = setUp('foo');
expect(result.error).toBeUndefined();
});

it('should init getter and setter', () => {
const { result } = setUp('foo');
const [get, set] = result.current;
expect(get).toBeInstanceOf(Function);
expect(set).toBeInstanceOf(Function);
});

it('should get current value', () => {
const { result } = setUp('foo');
const [get] = result.current;
expect(get()).toBe('foo');
});

it('should set new value', () => {
const { result } = setUp('foo');
const [get, set] = result.current;
act(() => set('bar'));
const currentValue = get();
expect(currentValue).toBe('bar');
});

it('should get and set expected values when used in nested functions', () => {
const { result } = setUp(0);
const [get, set] = result.current;
const onClick = jest.fn(() => {
setTimeout(() => {
set(get() + 1);
}, 1000);
});

// simulate 3 clicks
onClick();
onClick();
onClick();

act(() => {
jest.runAllTimers();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

it is way overcomplicated, why not to simply set state inside act?

act(()=>{
 set();
 set();
 set();
})


expect(get()).toBe(3);
expect(onClick).toHaveBeenCalledTimes(3);
});
});
62 changes: 62 additions & 0 deletions src/useGetSet/__tests__/ssr.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { renderHook, act } from '@testing-library/react-hooks/server';
import { useGetSet } from '../..';

const setUp = (initialValue: any) => renderHook(() => useGetSet(initialValue));

beforeEach(() => {
jest.useFakeTimers();
});

describe('useGetSet', () => {
it('should be defined', () => {
expect(useGetSet).toBeDefined();
});

it('should render', () => {
const { result } = setUp('foo');
expect(result.error).toBeUndefined();
});

it('should init getter and setter', () => {
const { result } = setUp('foo');
const [get, set] = result.current;
expect(get).toBeInstanceOf(Function);
expect(set).toBeInstanceOf(Function);
});

it('should get current value', () => {
const { result } = setUp('foo');
const [get] = result.current;
expect(get()).toBe('foo');
});

it('should set new value', () => {
const { result } = setUp('foo');
const [get, set] = result.current;
act(() => set('bar'));
const currentValue = get();
expect(currentValue).toBe('bar');
});

it('should get and set expected values when used in nested functions', () => {
const { result } = setUp(0);
const [get, set] = result.current;
const onClick = jest.fn(() => {
setTimeout(() => {
set(get() + 1);
}, 1000);
});

// simulate 3 clicks
onClick();
onClick();
onClick();

act(() => {
jest.runAllTimers();
});

expect(get()).toBe(3);
expect(onClick).toHaveBeenCalledTimes(3);
});
});
19 changes: 19 additions & 0 deletions src/useGetSet/useGetSet.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { useRef, useCallback } from 'react';
import { IInitialState, INextState, resolveHookState } from '../util/resolveHookState';
import { useRerender } from '../index';

export function useGetSet<S>(
initialState: IInitialState<S>
): [get: () => S, set: (nextState: INextState<S>) => void] {
const rerender = useRerender();
const ref = useRef<S>(resolveHookState(initialState));
const get = useCallback(() => ref.current, []);
const set = useCallback(
(nextState: INextState<S>) => {
ref.current = resolveHookState(nextState, ref.current);
rerender();
},
[rerender]
);
return [get, set];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to use useSafeState, useSyncedRef and useCallback?

Besides this code will work - id prefer native state holder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't understand here.

id prefer native state holder

what is it means here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

either useState nor useSafeState update the value immediately. That why this hook is need and useRerender is used in setter to control UI updates.

How useSyncedRef can be used here

Copy link
Contributor

@xobotyi xobotyi Dec 16, 2021

Choose a reason for hiding this comment

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

This implementation breaks 'state flow' of react, and will break in strict mode, 4ex.

What i propose is to implement hook like this:

export function useGetSet<S>(
  initialState: IInitialState<S>
): [get: () => S, set: Dispatch<SetStateAction<S>>] {
  const [state, setState] = useSafeState(initialState);
  const stateRef = useSyncedRef(state);

  // eslint-disable-next-line react-hooks/exhaustive-deps
  return [useCallback(() => stateRef.current, []), setState];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation doesn't return latest value in getter. /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But how to write test, since I don't know when will react state change

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wrap all set calls with act method of testing library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, PR lack of readme changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wrap all set calls with act method of testing library.

  it('should set new value', () => {
    const { result } = setUp('foo');
    const [get, set] = result.current;
    act(() => set('bar'));
    const currentValue = get();
    expect(currentValue).toBe('bar');
  });

This test cannot pass /

}