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

Introduce NetworkConfig API #449

Merged
merged 23 commits into from
Jun 3, 2024

Conversation

123mpozzi
Copy link
Contributor

@123mpozzi 123mpozzi commented May 27, 2024

Description

Introduce NetworkConfig with two APIs for pre-processing HTTP requests and responses.

PRs planning 📝

This PR contains the public API in typescript, and the implementation of one of the two APIs (preprocessHttpResponse).

At least three more PRs will follow. Specifically: #452 to implement the APIs in Android, #450 to implement the second API in iOS, and one to cover the API with system tests.

However, all PRs will not be merged into development until they're all ready. In fact we will use a base branch for this feature.

Changes ✍️

Typescript

Introduce the NetworkConfig API, accessible through playerConfig.networkConfig

Config definition in network/networkConfig.ts

This file contains the config type definition together with its other related types.

NetworkConfig:

  • preprocessHttpRequest(type, request) => Promise<HttpRequest> to pre-process an HTTP request before the request is made
  • preprocessHttpResponse(type, response) => Promise<HttpResponse> to pre-process an HTTP response before it gets into the player

Other relevant types

  • Notably, HttpRequestType has been moved here from src/events.ts

Introduce Network module in network/index.ts

Handles the Network module and binds the actual native calls to JS through custom listeners.

Why a module? We need a module here because to serialise callbacks we have to call functions on a platform from the other.
Note: this is very similar to what we did for drm/index.ts, although there is no asynchronousity in those callbacks.

iOS

  • Implement the network module to handle the bridging to and from JS. The bridging is used for passing the config object and running the callback on the JS side
  • Add serialisers for network config types

Implementation details 🔬 (*)

The main struggle here was serialising a config object containing two APIs which are both async callbacks. We went for the following approach:

  • Create a native module to make the JS code callable from native
  • On native side, initialise the native configuration object by fetching the JS one (e.g. using json format)
    • The config will be initialised as an empty shell initially as the callbacks cannot be serialised directly
  • Check if the wanted API is defined in the fetched json config. If so, start defining the native callback: serialise the parameters needed by the native API to a JS-friendly format (1), and call the JS method responsible of running the callback using the params
  • On the JS side, the method gets the serialised parameters and applies the callback (2). Finally it gets the eventual resulting object and sends it back to the native side
  • The native side can now acknowledge the fact that the callback has been called on the JS side, and hence runs any additional logic (3)

(1) In this case the API has a one of its parameter being a completion handler, which is required to finalise the pre-processing. So we also assign an identifier to the response to keep track of it. We store the completion handlers on the native side
(2) In this case, given that the callback is async, this method also handles the resolving and rejection of the promise
(3) In this case it calls the completion handler assigned to the response

Reading this additional section may be helpful for serialising callbacks in future PRs

Testing preprocessHttpResponse (iOS) 🧪

Use httpCallbacks.patch which modifies both BasicPlayback and DrmPlayback.
To test for a non-empty body field, use DRM stream samples.

However note that, in this PR, the request callback is not yet implemented, so it will not log or do anything!

And finally: don't forget to run yarn bootstrap!

Things to look for:

  • fields are serialised correctly
    • not randomly null
    • values make sense
  • performance is fine

Checklist

  • 🗒 CHANGELOG entry

@123mpozzi 123mpozzi self-assigned this May 27, 2024
@123mpozzi 123mpozzi requested a review from a team as a code owner May 27, 2024 12:05
@123mpozzi 123mpozzi marked this pull request as draft May 27, 2024 12:07
@123mpozzi 123mpozzi removed the request for review from a team May 27, 2024 12:08
@123mpozzi 123mpozzi changed the title Introduce Network API and Config Introduce NetworkConfig API May 27, 2024
@123mpozzi 123mpozzi force-pushed the feature/introduce-networkConfig branch from 6258f52 to a3932c0 Compare May 27, 2024 14:24
@123mpozzi 123mpozzi force-pushed the feature/introduce-networkConfig branch from a3932c0 to 69d0930 Compare May 27, 2024 14:31
@123mpozzi 123mpozzi changed the title Introduce NetworkConfig API Introduce NetworkConfig API May 27, 2024
return nil
}
return NetworkConfig()
}
Copy link
Contributor Author

@123mpozzi 123mpozzi May 27, 2024

Choose a reason for hiding this comment

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

This func has no logic with the json because, as of now, the only things to serialise inside NetworkConfig are callbacks, which have to be set up in a different way, and in their corresponding module.


if let bodyBase64EncodedString = json["body"] as? String {
request.body = Data(base64Encoded: bodyBase64EncodedString)
}
Copy link
Contributor Author

@123mpozzi 123mpozzi May 27, 2024

Choose a reason for hiding this comment

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

We are serialising a Data / NSData object using base64-encoded strings

@123mpozzi 123mpozzi changed the base branch from development to feature/base-networkConfig May 28, 2024 07:58
.then((resultResponse) => {
NetworkModule.setPreprocessedHttpResponse(responseId, resultResponse);
});
};
Copy link
Contributor Author

@123mpozzi 123mpozzi May 28, 2024

Choose a reason for hiding this comment

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

The other API will be part of an additional PR, but it will most likely follow the same pattern

123mpozzi added 6 commits May 28, 2024 14:56
As this is basically the body of a closure, exported into a function
for clarity.
And remove drm headers from code samples as it
may be confusing
@123mpozzi 123mpozzi marked this pull request as ready for review May 28, 2024 14:12
@123mpozzi 123mpozzi requested review from rolandkakonyi and a team May 28, 2024 14:13
@123mpozzi
Copy link
Contributor Author

123mpozzi commented May 28, 2024

I am adding an Android reviewer to look at the public API changes in typescript files (and the changelog)

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
ios/NetworkModule.swift Outdated Show resolved Hide resolved
ios/NetworkModule.swift Outdated Show resolved Hide resolved
ios/RCTConvert+BitmovinPlayer.swift Outdated Show resolved Hide resolved
src/network/networkConfig.ts Outdated Show resolved Hide resolved
src/network/networkConfig.ts Outdated Show resolved Hide resolved
@123mpozzi 123mpozzi requested a review from rolandkakonyi May 29, 2024 11:58
123mpozzi added 2 commits May 29, 2024 15:52
As this will be aligned with the upcoming iOS extension PR
@123mpozzi 123mpozzi merged commit f6d1ae5 into feature/base-networkConfig Jun 3, 2024
7 checks passed
@123mpozzi 123mpozzi deleted the feature/introduce-networkConfig branch June 3, 2024 12:51
@123mpozzi 123mpozzi mentioned this pull request Jun 3, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants