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

Make sure Firestore hooks return undefined when data does not exist #446

Merged
merged 8 commits into from
Sep 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
14 changes: 7 additions & 7 deletions docs/reference/classes/SuspenseSubject.SuspenseSubject-1.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion docs/reference/modules/useObservable.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
"babel-jest": "^26.6.3",
"babel-plugin-minify-replace": "^0.5.0",
"eslint-plugin-no-only-tests": "^2.6.0",
"firebase": "^9.0.0",
"firebase": "^9.0.1",
"firebase-tools": "^9.16.0",
"globalthis": "^1.0.1",
"husky": "^4.3.0",
Expand Down
6 changes: 4 additions & 2 deletions src/SuspenseSubject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,16 @@ export class SuspenseSubject<T> extends Subject<T> {
return this._hasValue || !!this._error;
}

get value(): T | undefined {
get value(): T {
// TODO figure out how to reset the cache here, if I _reset() here before throwing
// it doesn't seem to work.
// As it is now, this will burn the cache entry until the timeout fires.
if (this._error) {
throw this._error;
} else if (!this.hasValue) {
throw Error('Can only get value if SuspenseSubject has a value');
}
return this._value;
return this._value as T;
}

get firstEmission(): Promise<void> {
Expand Down
76 changes: 48 additions & 28 deletions src/useObservable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,55 +63,75 @@ export interface ObservableStatus<T> {
firstValuePromise: Promise<void>;
}

function reducerFactory<T>(observable: SuspenseSubject<T>) {
return function reducer(state: ObservableStatus<T>, action: 'value' | 'error' | 'complete'): ObservableStatus<T> {
// always make sure these values are in sync with the observable
const newState = {
...state,
hasEmitted: state.hasEmitted || observable.hasValue,
error: observable.ourError,
firstValuePromise: observable.firstEmission,
};
if (observable.hasValue) {
newState.data = observable.value;
}

switch (action) {
case 'value':
newState.status = 'success';
return newState;
case 'error':
newState.status = 'error';
return newState;
case 'complete':
newState.isComplete = true;
return newState;
default:
throw new Error(`invalid action "${action}"`);
}
};
}

export function useObservable<T = unknown>(observableId: string, source: Observable<T>, config: ReactFireOptions = {}): ObservableStatus<T> {
// Register the observable with the cache
if (!observableId) {
throw new Error('cannot call useObservable without an observableId');
}
const observable = preloadObservable(source, observableId);

// Suspend if suspense is enabled and no initial data exists
const hasInitialData = config.hasOwnProperty('initialData') || config.hasOwnProperty('startWithValue');

const hasData = observable.hasValue || hasInitialData;
const suspenseEnabled = useSuspenseEnabledFromConfigAndContext(config.suspense);

if (suspenseEnabled === true && !observable.hasValue && (!config?.initialData ?? !config?.startWithValue)) {
if (suspenseEnabled === true && !hasData) {
throw observable.firstEmission;
}

const [latest, setValue] = React.useState(() => (observable.hasValue ? observable.value : config.initialData ?? config.startWithValue));
const [isComplete, setIsComplete] = React.useState(false);
const [hasError, setHasError] = React.useState(false);
const initialState: ObservableStatus<T> = {
status: hasData ? 'success' : 'loading',
hasEmitted: hasData,
isComplete: false,
data: observable.hasValue ? observable.value : config?.initialData ?? config?.startWithValue,
error: observable.ourError,
firstValuePromise: observable.firstEmission,
};
const [status, dispatch] = React.useReducer<React.Reducer<ObservableStatus<T>, 'value' | 'error' | 'complete'>>(reducerFactory<T>(observable), initialState);

React.useEffect(() => {
const subscription = observable.subscribe({
next: (v) => {
setValue(() => v);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where the original issue was. Since value was undefined when the state was set up, calling setValue(() => undefined) meant that React didn't trigger a re-render, since value didn't change.

next: () => {
dispatch('value');
},
error: (e) => {
setHasError(true);
dispatch('error');
throw e;
},
complete: () => {
setIsComplete(true);
dispatch('complete');
},
});
return () => subscription.unsubscribe();
}, [observable]);

let status: ObservableStatus<T>['status'];

if (hasError) {
status = 'error';
} else if (observable.hasValue || hasInitialData) {
status = 'success';
} else {
status = 'loading';
}

return {
status,
hasEmitted: observable.hasValue || hasInitialData,
isComplete: isComplete,
data: latest,
error: observable.ourError,
firstValuePromise: observable.firstEmission,
};
return status;
}
17 changes: 17 additions & 0 deletions test/firestore.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,23 @@ describe('Firestore', () => {
expect(data.a).toEqual(mockData.a);
expect(data.id).toBeDefined();
});

it('returns undefined if document does not exist', async () => {
const collectionRef = collection(db, randomString());
const docIdThatExists = randomString();
const docIdThatDoesNotExist = randomString();
await setDoc(doc(collectionRef, docIdThatExists), { a: randomString() });

// reference a doc that doesn't exist
const ref = doc(collectionRef, docIdThatDoesNotExist);

const { result, waitFor } = renderHook(() => useFirestoreDocData<any>(ref, { idField: 'id' }), { wrapper: Provider });

await waitFor(() => result.current.status === 'success');

expect(result.current.status).toEqual('success');
expect(result.current.data).toBeUndefined();
});
});

describe('useFirestoreDocOnce', () => {
Expand Down
13 changes: 13 additions & 0 deletions test/useObservable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,19 @@ describe('useObservable', () => {
expect(result.current.data).toEqual(startVal);
});

it('emits even if data is undefined', async () => {
const observable$: Subject<any> = new Subject();

const { result } = renderHook(() => useObservable('test-undefined', observable$, { suspense: false }));

expect(result.current.status).toEqual('loading');

actOnHook(() => observable$.next(undefined));

expect(result.current.status).toEqual('success');
expect(result.current.data).toBeUndefined();
});

it('does not show stale data after navigating away', async () => {
const startVal = 'start';
const newVal = 'anotherValue';
Expand Down
Loading