Synchronization option now supports syncUpdateCondition to skip remote record

[primus11]

Atm synchronization only allows to customize conflict resolution which doesn't provide full control when trying for example to skip remote record altogether. It is possible to use conflict resolution that would return local but this is happening too late and has side effect of modifying updated_at.

edit - some more notes:

It is currently problematic to achieve idempotency when doing repetitive pullChanges. For example I would expect below code to be no-op but since potential local change will be resolved our updated_at will be updated. I am aware that custom "updated_at" could be used instead but that feels more like working against WatermelonDB.

function conflictResolver(
	tableName: TableName<any>,
	local: DirtyRaw,
	remote: DirtyRaw,
	resolved: DirtyRaw,
): DirtyRaw {
	const localUpdatedAt = local.updated_at;
	if (localUpdatedAt == null) {
		return remote;
	}

	const remoteUpdatedAt = remote.updated_at;

	if (localUpdatedAt > remoteUpdatedAt) {
		return local;
	} else {
		return remote;
	}
}

[radex]

Hi, this makes sense to me. I propose changing this to work more similarly to to conflictResolver - i.e. first the built-in requiresUpdate runs, then the result of that is passed to a custom shouldUpdateRecord? function. This way, it's going to be easier to use this correctly - override the default behavior just in specific cases, but leave the built-in optimization on otherwise.

For example I would expect below code to be no-op but since potential local change will be resolved our updated_at will be updated

BTW I'm not sure if this is correct. It's arguable whether this is expected behavior, but I think that's a bug. I specifically added doesn't touch created_at/updated_at when applying updates test

[primus11]

I can move it yeah if that is preferred. My thinking at the end was that it maybe belongs to requiresUpdate and that if possible it is better that it is done before sanitizedRow and areRecordsEqual in it. Atm at least, it can also be before built-in requiresUpdate. Please just reconfirm and I will also change naming to shouldUpdateRecord at that point.

BTW I'm not sure if this is correct. It's arguable whether this is expected behavior, but I think that's a bug. I specifically added doesn't touch created_at/updated_at when applying updates test

This puzzled me a bit as well. The thing is that areRecordsEqual is triggered before conflictResolver takes place which automatically means that this record will go into prepareUpdate at that point. My first thinking was to add control when updated_at is updated since I believe it could be used elsewhere but it would be a bit funky from prepareUpdateFromRaw perspective and this change just achieves wanted result a lot better.

Edit: Maybe areRecordsEqual should be moved but it could potentially be worse performance-wise for some

[radex]

that if possible it is better that it is done before sanitizedRow and areRecordsEqual in it

Ok, rename it to shouldUpdateRecord, but keep the arguments as-is. We could export the original implementation (possibly renamed for consistency) so that one can use that.

[primus11]

    it('forced local should keep record intact', async () => {
      const { database, comments } = makeDatabase()

      await makeLocalChanges(database)
      await testApplyRemoteChanges(database, {
        mock_comments: {
          updated: [{ id: 'cSynced', body: 'remote' }],
        },
      }, {
        conflictResolver: (
          tableName: TableName<any>,
          local: DirtyRaw,
          remote: DirtyRaw,
          resolved: DirtyRaw,
        ) => {
          return local;
        }
      })

      await expectSyncedAndMatches(comments, 'cSynced', {
        _status: 'synced',
        _changed: '',
      })
    })

will cause

 FAIL  src/sync/impl/__tests__/applyRemote.test.js (32.296 s)
   applyRemoteChanges  conflictResolver  forced local should keep record intact

    expect(received).toMatchObject(expected)

    - Expected  - 2
    + Received  + 2

      Object {
    -   "_changed": "",
    -   "_status": "synced",
    +   "_changed": "updated_at",
    +   "_status": "updated",