Bubble up errors while loading Resources

[ettore]

Errors are bubbled up to the app via a new NavigatorDelegate method:

navigator(_:didFailToLoadResourceAt:url:withError)

No breaking changes to the Navigators client-facing APIs.

This solves issue https://github.com/readium/swift-toolkit/issues/398.

The 2nd commit depends on PR https://github.com/readium/GCDWebServer/pull/11. It is not strictly required but it is nice to have, because it attaches the underlying error to the GCDWebServerErrorResponse.

[ettore]

ugh the build fails because of that GCDWebServer PR. Let me know what you think of it, and if needed I can just nuke the 2nd commit in this PR.

[ettore]

The changes to HTTPServer are breaking the semantic versioning. We could either switch the base branch to v3, or maintain the old HTTPServer APIs with fallbacks. What do you prefer?

🤔 I have not looked into v3 so I don't know what changes it entails on our end. If migrating to v3 is not going to be a big effort, maybe that's doable.

For the other option, do you mean having HTTPServer include both versions of the api, meaning:

// new
func serve(at endpoint: HTTPServerEndpoint, 
           handler: @escaping (HTTPServerRequest) -> Resource,
           failureHandler: FailureHandler?) throws -> URL
// old
func serve(at endpoint: HTTPServerEndpoint, handler: @escaping (HTTPServerRequest) -> Resource) throws -> URL

where basically the old one simply calls the new one with a nil failureHandler (perhaps in a protocol extension)?

I plan to release at least one other minor version from develop before v3 is released.

👍 In this case the 2nd option with fallbacks might be better. It would be preferable for us to be able to use this new stuff sooner rather than later. (And I'd prefer to avoid targeting develop for our production builds 😅 )

[mickael-menu]

That's right, keeping the old APIs and just forwarding the calls to the new ones could be enough here. 👌

[ettore]

I am looking into the errors on the "Local" and "SPM" checks, but the build log is truncated and very hard to browse. I have a hunch these errors are maybe on the TestApp (which I am no longer using), because the actual toolkit compiles fine locally.

I wonder if the build logs could be improved, perhaps with xcpretty? we've been using it and it makes the build logs much clearer because it highlights the errors and it is more succinct for the commands that succeed. IIRC it is provided by GitHub out of the box, so I could try adding it to the xcodebuild commands.

[ettore]

I was able to build the TestApp and fix all the regressions 😅 I also fixed a couple easy warnings.

I ended up adding xcpretty since it was very simple to do so and the benefits are big. There's a night and day difference in how the logs are easier to read (before | after). It did help in diagnosing the problem from the logs, which was not possible before. There's some script-code duplication in the workflow, but hopefully that can be done at some other time.

[ettore]

• I changed the NavigatorDelegate API to /.../ require a non optional href, as it identifies a particular resource. This is to be consistent with the same Kotlin API.

I tested my code with this change and the errors I was observing are still being reported, so this should be ok, I think. But I do worry though that the failureHandler calls where the HTTPServerRequest::href is nil (here, here and here) are now not going to reach the navigator side since we filter for a non-optional href.

[mickael-menu]

But I do worry though that the failureHandler calls where the HTTPServerRequest::href is nil (here, here and here) are now not going to reach the navigator side since we filter for a non-optional href.

These are outside the scope of broken publication resources, so they should not be reported with NavigatorDelegate.didFailToLoadResourceAt. If they occur, there's probably something wrong in the app itself which should be fixed by the integrator.

Although I would be open to add a global fallback failure handler directly in the GCDHTTPServer type, that applications could provide when creating the server.

[ettore]

These are outside the scope of broken publication resources, so they should not be reported with NavigatorDelegate.didFailToLoadResourceAt. If they occur, there's probably something wrong in the app itself which should be fixed by the integrator.

Although I would be open to add a global fallback failure handler directly in the GCDHTTPServer type, that applications could provide when creating the server.

I was even just thinking something like another "catch all" delegate method to do something like this

guard let href = href else {
    self.delegate?.epubNavigatorViewModel(self, didFailWithError: error)
    return
}
self.delegate?.epubNavigatorViewModel(self, didFailToLoadResourceAt: href, withError: error)

I did not propose it because it's still unclear to me when the case of a nil href actually happens. In any case I agree this is beyond the scope of Resource loading fails. We'd also be risking delegate callback pollution. 😅

Thanks for merging the PR! 🎉