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

Implement useReactiveVar hook for consuming reactive variables in React components. #6867

Merged
merged 6 commits into from
Aug 20, 2020
Merged
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
Prev Previous commit
Next Next commit
Avoid visiting new listeners added by other listeners.
A common pitfall when using forEach to iterate over a set of callback
functions, thankfully caught by the tests I wrote.
  • Loading branch information
benjamn committed Aug 20, 2020
commit b9382b07df1e74a3f634dd6c6805f80bd73a5a2d
18 changes: 12 additions & 6 deletions src/cache/inmemory/reactiveVars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ const varDep = dep<ReactiveVar<any>>();
// called in Policies#readField.
export const cacheSlot = new Slot<ApolloCache<any>>();

// A listener function could in theory cause another listener to be added
// to the set while we're iterating over it, so it's important to commit
// to the original elements of the set before we begin iterating. See
// iterateObserversSafely for another example of this pattern.
function consumeAndIterate<T>(set: Set<T>, callback: (item: T) => any) {
const items: T[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

const items = [...set] ?

Copy link
Member Author

@benjamn benjamn Aug 20, 2020

Choose a reason for hiding this comment

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

You have every reason to expect that to work, but TypeScript complains:

Type 'Set<T>' is not an array type or a string type. Use compiler option '--downlevelIteration' to allow iterating of iterators. [2569]

The ugly truth is that --downlevelIteration adds overhead to all ...spread operations (even the Array-only ones), so that's unfortunately not an option for us, until we're comfortable dropping support for Internet Explorer.

set.forEach(item => items.push(item));
set.clear();
items.forEach(callback);
}

export function makeVar<T>(value: T): ReactiveVar<T> {
const listeners = new Set<ReactiveListener<T>>();

Expand All @@ -24,12 +35,7 @@ export function makeVar<T>(value: T): ReactiveVar<T> {
if (value !== newValue) {
value = newValue!;
varDep.dirty(rv);
listeners.forEach(listener => {
// Listener functions listen only for the next update, not all
// future updates.
listeners.delete(listener);
listener(value);
});
consumeAndIterate(listeners, listener => listener(value));
}
} else {
// When reading from the variable, obtain the current cache from
Expand Down