Skip to content
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
6 changes: 6 additions & 0 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,12 @@ function keysChanged(collectionKey, collection) {
if (isSubscribedToCollectionKey) {
if (_.isFunction(subscriber.callback)) {
const cachedCollection = getCachedCollection(collectionKey);

if (subscriber.waitForCollectionCallback) {
subscriber.callback(cachedCollection);
return;
}

_.each(collection, (data, dataKey) => {
subscriber.callback(cachedCollection[dataKey], dataKey);
});
Expand Down
48 changes: 39 additions & 9 deletions tests/unit/onyxTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const ONYX_KEYS = {
COLLECTION: {
TEST_KEY: 'test_',
TEST_CONNECT_COLLECTION: 'test_connect_collection_',
TEST_POLICY: 'test_policy_',
},
};

Expand Down Expand Up @@ -470,11 +471,10 @@ describe('Onyx', () => {
});

it('should return all collection keys as a single object when waitForCollectionCallback = true', () => {
const valuesReceived = {};
const mockCallback = jest.fn(data => valuesReceived[data.ID] = data.value);
const mockCallback = jest.fn();

// GIVEN some collection data
const collectionData = {
// GIVEN some initial collection data
const initialCollectionData = {
test_connect_collection_1: {
ID: 123,
value: 'one',
Expand All @@ -488,10 +488,11 @@ describe('Onyx', () => {
value: 'three',
},
};
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_CONNECT_COLLECTION, collectionData);

Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_CONNECT_COLLECTION, initialCollectionData);
return waitForPromisesToResolve()
.then(() => {
// WHEN we call Onyx.connect with the waitForCollectionCallback = true param
// WHEN we connect to that collection with waitForCollectionCallback = true
connectionID = Onyx.connect({
key: ONYX_KEYS.COLLECTION.TEST_CONNECT_COLLECTION,
waitForCollectionCallback: true,
Expand All @@ -500,11 +501,40 @@ describe('Onyx', () => {
return waitForPromisesToResolve();
})
.then(() => {
// THEN the callback should be triggered only once
// THEN we expect the callback to be called only once and the initial stored value to be initialCollectionData
expect(mockCallback.mock.calls.length).toBe(1);
expect(mockCallback.mock.calls[0][0]).toEqual(initialCollectionData);
Comment thread
luacmartins marked this conversation as resolved.
});
});

it('should return all collection keys as a single object when updating a collection key with waitForCollectionCallback = true', () => {
const mockCallback = jest.fn();
const collectionUpdate = {
test_policy_1: {ID: 234, value: 'one'},
test_policy_2: {ID: 123, value: 'two'},
};

// GIVEN an Onyx.connect call with waitForCollectionCallback=true
connectionID = Onyx.connect({
key: ONYX_KEYS.COLLECTION.TEST_POLICY,
waitForCollectionCallback: true,
callback: mockCallback,
});
return waitForPromisesToResolve()
.then(() => {
// WHEN mergeCollection is called with an updated collection
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_POLICY, collectionUpdate);
return waitForPromisesToResolve();
})
.then(() => {
// THEN we expect the callback to have called twice, once for the initial connect call + once for the collection update
expect(mockCallback.mock.calls.length).toBe(2);

// AND the value for the first call should be null since the collection was not initialized at that point

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does it seem weird to anyone else that:

  1. There would be two calls
  2. The first call passes null
  3. The second call is really what we want with waitForCollectionCallback=true

?

Is it possible for us to remove that first call? What is the value in having the call triggered with null before the collection has been "initialized"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is expected since we want to initialize that key's value to null, since we are connecting to that key, we just don't have a value for it. We leave it up to the subscriber to decide what to do with a null value.

This happens here before we run any callback subscriber logic. I think it would make sense to keep the behavior consistent.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 to what @luacmartins said :), I would also prefer to avoid exceptions and let the subscriber handle that. What if null is a valid value and we want to be notified about it?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, those are good points. I think it would be a change in behavior that could make other bugs in the process, so this was more of an exploratory question. Let's leave it as-is!

expect(mockCallback.mock.calls[0][0]).toBe(null);

// AND all the collection data should be returned as a single object
expect(mockCallback.mock.calls[0][0]).toEqual(collectionData);
// AND the value for the second call should be collectionUpdate since the collection was updated
expect(mockCallback.mock.calls[1][0]).toEqual(collectionUpdate);
});
});
});