Skip to content

useMutableSource: Allow returning function from getSnapshot #18921

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

Closed
wants to merge 1 commit into from
Closed

useMutableSource: Allow returning function from getSnapshot #18921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,8 @@ function useMutableSource<Source, Snapshot>(
const latestSetSnapshot = refs.setSnapshot;

try {
latestSetSnapshot(latestGetSnapshot(source._source));
const value = latestGetSnapshot(source._source);
latestSetSnapshot(() => value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an issue that I chatted about with Daishi a few weeks back, which has this obvious fix but... I had hoped to avoid the [almost always] unnecessary function wrapper. Still don't know how I feel about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Andrew's comment just loaded in for me after writing mine.

I agree with the concern he mentions about opening up extra problem cases if the function itself closes over mutable values.

As an alternative, could we say we don't support functions as snapshots and warn in dev if we detect one?

I tend to like this better.


// Record a pending mutable source update with the same expiration time.
const suspenseConfig = requestCurrentSuspenseConfig();
Copy link
Collaborator Author

@sophiebits sophiebits May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

(nb: we still need to support some kind of unwrapping because of the exceptions a few lines below this – we could maybe make something different for that to avoid allocating the function in the success case, but… I feel like this is fine?)

Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,7 +1004,8 @@ function useMutableSource<Source, Snapshot>(
const latestSetSnapshot = refs.setSnapshot;

try {
latestSetSnapshot(latestGetSnapshot(source._source));
const value = latestGetSnapshot(source._source);
latestSetSnapshot(() => value);

// Record a pending mutable source update with the same expiration time.
const currentTime = requestCurrentTimeForUpdate();
Expand Down
39 changes: 39 additions & 0 deletions packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1493,6 +1493,45 @@ describe('useMutableSource', () => {
},
);

// @gate experimental
it(
'can store functions in source',
async () => {
const source = createSource(() => 'a');
const mutableSource = createMutableSource(source);

const getSnapshot = () => source.value;

function Read() {
const fn = useMutableSource(
mutableSource,
getSnapshot,
defaultSubscribe,
);
const value = fn();
Scheduler.unstable_yieldValue(value);
return value;
}

const root = ReactNoop.createRoot();
await act(async () => {
root.render(
<>
<Read />
</>,
);
});
expect(Scheduler).toHaveYielded(['a']);
expect(root).toMatchRenderedOutput('a');

await act(async () => {
source.value = () => 'b';
expect(Scheduler).toFlushUntilNextPaint(['b']);
expect(root).toMatchRenderedOutput('b');
});
},
);

// @gate experimental
it('getSnapshot changes and then source is mutated during interleaved event', async () => {
const {useEffect} = React;
Expand Down