Add a new Core Data write API

[crazytonyli]

Fixes #15821

This change is part of paaHJt-1I7-p2. This PR only introduces the new write API and doesn't update the app code to use it.

Two functions are introduced in ContextManager; one is a synchronous call, and the other is an asynchronous call. I'm aware this doesn't follow the existing pattern: saveContext: and saveContextAndWait:. The fact that saveUsingBlock: doesn't take a completion block implies it's synchronous. I'm keen to hear what you all think about these new functions. Does it simplify the API, or on the contrary, make the API confusing?

The implementation of both functions reuses the existing saving context method to ensure database changes are propagated through to the mainContext and writerContext.

Regression Notes

No regression needed, since no app code uses this new API yet.

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

[wpmobilebot]

You can test the changes in Jetpack from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr19169-5e20183 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[wpmobilebot]

You can test the changes in WordPress from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr19169-5e20183 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[AliSoftware]

I'm keen to hear what you all think about these new functions. Does it simplify the API, or on the contrary, make the API confusing?

I wonder if it might be worth adding some random WPiOS devs as reviewers to this PR to get their thoughts about this here 🙃

[crazytonyli]

@AliSoftware Very good point!

Hi @Gio2018 , @dvdchr , and @momo-ozawa , I'm not sure if you all are actively working on this codebase, please feel free to ignore this ping if you aren't. If you are, can you please have a look at this small PR? I would appreciate your thoughts on not having a "AndWait" function in the new writer APIs. Thanks! (Feel free to add others to review this PR).

[mokagio]

This is equivalent to [self saveContext:context andWait:YES withCompletionBlock:nil], from https://github.com/wordpress-mobile/WordPress-iOS/pull/19169/files#diff-2fe4dfa10c8e9b74a4ecdf11a6ec23b485a01a3132435c259f820bc0f19c06e0R121-R124.

Have you considered providing only one method, the one below this one, and decide whether to run sync or async based on the completion value—if nil sync, ...andWait:YES withCompletionBlock:nil] otherwise ...andWait:YES withCompletionBlock:completion].

From the PR description, I think having to method was intentional, to differentiate between sync and async mode:

the fact that saveUsingBlock: doesn't take a completion block implies it's synchronous. I'm keen to hear what you all think about these new functions. Does it simplify the API, or on the contrary, make the API confusing?

I wonder whether we do need to expose a sync version? In particular given we can now use await like you show in the tests.

I'm asking all this because the naming makes things a bit unclear. If we definitely need two options, then I'd suggest using different names. This would also remove the need for the comment about which overloads the Swift compiler picks from the tests.

[mokagio]

I wonder whether we do need to expose a sync version?

I'm probably missing a piece of the puzzle, but from a quick search it seems like the usage of the sync saving is limited:

[crazytonyli]

[...] and decide whether to run sync or async based on the completion value

I'm a bit hesitant to make this change. IMO, sync or async should be a basic nature of an function, which shouldn't get decided implicitly by an argument. Also, contextManager.save(block, completion: nil) still looks async to me—passing a nil completion block means the user doesn't care about the save result.

I wonder whether we do need to expose a sync version? In particular given we can now use await like you show in the tests.

await is still async though. To my knowledge, we can't turn an await function call into an synchronous call within in a non-async function, can we?

[mokagio]

IMO, sync or async should be a basic nature of an function, which shouldn't get decided implicitly by an argument. Also, contextManager.save(block, completion: nil) still looks async to me—passing a nil completion block means the user doesn't care about the save result.

Fair point.

await is still async though.

Sure, but, to my understanding it lets us write code as if it was synchronous. I think these two syntax should be effectively the same:

methodThatDoesStuffSync()
otherMethod()

// ---

await methodThatDoesStuffAsync()
otherMethod()

In both cases, otherMethod() will run when the operation run by the previous method finishes. I haven't actually tested this and never done any real world dev with async await, so I might be missing something.

[crazytonyli]

Yep, await does make code looks synchronous, which is the main reason why it's such a popular feature I think.

I'll extend your example code to explain what I mean by "we can't turn an await function call into an synchronous call within in a non-async function"

async function can only be called within async context:

func fetchStuff() async { // <- The function has to be `async`
    await methodThatDoesStuffAsync()
    otherMethod()
}

If we want to call an async function inside a non-async function, we'll need to do something like this

func fetchStuff() { // <- Not `async` anymore
    Task { // <- But we have to wrap the `await` call in a `Task`.
        await methodThatDoesStuffAsync()
        otherMethod()
    }
    anotherOne()
}

However, anotherOne is going to be called before otherMethod, since Task is executed asynchronously. This means fetchStuff is a function that runs asynchronously—"stuff" isn't fetched at the moment the function is returned.

Adding a return value to fetchStuff would make this "issue" more obvious:

func fetchStuff() -> DownloadedFile {
    var fetchedFile: DownloadedFile!
    Task { // <- But we have to wrap the `await` call in a `Task`.
        let file: DownloadedFile = await methodThatDoesStuffAsync()
        fetchedFile = file // ❗️compiling error.
    }
    return fetchedFile // It's impossible to get hold of an variable that's created in above `Task`
}

This is why I don't think the sync version can be removed, unless we turn the whole all the callers to async functions, which is a quite drastic change.

I hope I am wrong on this, which would make things much nicer—let's write async functions all the way. 😸

[mokagio]

😮 gotcha! Of course... Thank you for taking the time 🙇‍♂️

👌

Given the usage of the sync saving is limited, maybe one day we'll be able to remove the sync version by converting all those call sites to use the async one 😄 But that's definitely a "later on" / "nice to have" kind of thing.

[mokagio]

Approving to unblock.

I'm keen to keep chatting about whether to offer a sync version at all, but that's something that can happen afterwards. 😄