Override fetch for Altas App Services #6165

kneth · 2023-09-29T14:00:17Z

What, How & Why?

This closes #6159

☑️ ToDos

📝 Changelog entry
📝 Compatibility label is updated or copied from previous entry
📝 Update COMPATIBILITY.md
🚦 Tests
📦 Updated internal package version in consuming package.jsons (if updating internal packages)
📱 Check the React Native/other sample apps work if necessary
💥 Breaking label has been applied or is not necessary

coveralls-official · 2023-09-29T14:06:06Z

coverage: 85.347% (-0.04%) from 85.388% when pulling e721b8f on kneth/custom-network-transport into 190349b on main.

kraenhansen · 2023-10-02T10:59:25Z

integration-tests/tests/src/hooks/import-app-before.ts

@@ -32,6 +32,7 @@ export type AppConfigurationRelaxed = {
  timeout?: number;
  multiplexSessions?: boolean;
  baseFilePath?: string;
+  fetchOverride?: any;


My immediate reaction is that the "override" part of this name is redundant as all of these options override default values.

Suggested change

fetchOverride?: any;

fetch?: any;

Additionally, would also be good to match the TS type here to whichever type is used on the SDK-exported type in App.ts.

kraenhansen · 2023-10-02T11:01:43Z

integration-tests/tests/src/node/custom-fetch.ts

+  const HTTP_METHOD: Record<number, string> = {
+    [0]: "GET",
+    [1]: "POST",
+    [2]: "PUT",
+    [3]: "PATCH",
+    [4]: "DELETE",
+  };


I wonder if it'll make more sense to export this somehow.

Maybe, but I have a feeling that we might leak too many implementation details.

kraenhansen · 2023-10-02T11:02:18Z

integration-tests/tests/src/node/custom-fetch.ts

+  const customFetch = async (request: Request) => {
+    called = true;
+    const { url, body, ...rest } = request;
+    if (typeof request.method == "number") {
+      rest.method = HTTP_METHOD[request.method];
+    }
+    const response = await fetch(url, rest);
+    return response;
+  };


An alternative could be to implement this as a wrapping of the default fetch from @realm/network-transport.

I am split here: part of me wants to wrap it as much as possible, but a part of me wishes to give the user full flexibility.

I think this solution is good, otherwise we would also have to provide a mechanism to override the default AbortController as well. This basically lets the user override @realm/network-transport.
I guess the crux of the issue, is the user that reported the original issue is using electron and this is using the node bindings instead of the browser bindings to fetch.

takameyer · 2023-10-16T13:09:51Z

packages/realm/realm-constants.json

@@ -1 +1 @@
-{"REALM_ANONYMIZED_BUNDLE_ID":"tkgif/+3l1e9wStGJp2TOngAK3UcQ2u7OM8ZYJU5JYo="}


Should this be reverted?

takameyer · 2023-10-16T13:13:52Z

packages/realm/src/app-services/App.ts

+   * @since 12.3.0
+   * @experimental This API is experimental and may change or be removed.
+   */
+  fetchOverride?: unknown;


Should we specify this type to match the NetworkType from network.ts

fetch(request: Request | binding.Request ): Promise<FetchResponse>;

I think it should be similar to a specification, or if that is too specific for a custom fetch, perhaps just a function type such as () => Promise<unknown>. Also, I'd say we should try to not expose bindgen details to the public API.

Will there be unintended side effects if the fetch function doesn't match our specifications?

Are you referring to our NetworkType? Since our TS expects the fetch used (whether it'd be ours or a user-provided one) to be any of the functions defined in the NetworkType, the preferable option (from the way it's set up atm) seems to be fetch(request: Request): Promise<FetchResponse>;. In that case we'd need to export Request and FetchResponse.

But, safest would probably be to ask the user of their intended use (arg and return type) and if fetch(request: Request): Promise<FetchResponse>; would be sufficient for them.

The user in this case just wants to replace node-fetch with window.fetch, since they are running realm in a browser environment in electron.

My main concern is this line. Here there is a transformation of the request happening depending on its contents. If a user is providing a custom fetch, will not doing this transformation cause an issue?

From this observation, perhaps @kraenhansen is correct in that we should directly override the fetch method within @realm/network-transport (a level deeper), and leave this one alone. Then we would also need to provide mechanisms to override the AbortController.

It's probably worth while doing a manual test in React Native, to see if such an override could cause significant issues.

I like it! 😎 I'll add it to our Round Table and let's discuss more :)

export function overrideFetch(fetch: (request: Request) => Promise<FetchResponse>) { baseFetch = fetch; }

My immediate feeling is that I like the declarative API of keeping this a configuration of the App. Possibly providing a way to override the default values for those config values, but then again - how many developers do we expect to have more than one App instance per application?

I like ☝️ over this more procedural overrideFetch function suggestion as that minimize the state in the library, which also has the added benefit of allowing two App instances to fetch using different implementations concurrently.

The intention of the overrideFetch suggestion keeps the configuration through App, not for the developer to call this directly. The point being, to keep the internal logic of network:fetch (calls to toFetchRequest), and give the opportunity to override the fetch within the network:fetch method.
I was not proposing to remove the configuration by App, but to clear up that we would be overriding the internal network:fetch logic with the current PR state.

@takameyer I don't see how that'll work. The App (or something with access to the fetch passed via the App's configuration) would have to call overrideFetch, passing in the fetch before every single request to the server, as it'll be a shared state between two App instances, that would have to be constantly switched between the two.

@kraenhansen Then we should write this in a way that works in the case you are suggesting. The code I wrote above was only an example, but the intention should be clear.

In any case, I think we are misunderstanding each other and would enjoy discussing this further when you return.

elle-j

Great digging, thanks for taking the time to solve this! Left some comments and suggestions to hopefully improve it even more.

elle-j · 2023-10-16T15:55:05Z

CHANGELOG.md

@@ -4,7 +4,28 @@
 * `Realm.User.providerType` is deprecated, and will be remove in next major version. Use `Realm.User.identities` instead.

 ### Enhancements
-* None
+* Expose `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))


I'm wondering if we should say AppConfiguration.fetchOverride rather than Realm.App.Configuration.fetchOverride due to how users currently can import it. Atm it seems like they can only import it via { AppConfiguration }.

Suggested change

* Expose `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))

* Exposed `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))

elle-j · 2023-10-16T15:56:03Z

CHANGELOG.md

-* None
+* Expose `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))
+
+```js


Suggested change

```js

```typescript

elle-j · 2023-10-16T16:02:51Z

integration-tests/tests/src/hooks/import-app-before.ts

@@ -32,6 +32,7 @@ export type AppConfigurationRelaxed = {
  timeout?: number;
  multiplexSessions?: boolean;
  baseFilePath?: string;
+  fetchOverride?: any;


Additionally, would also be good to match the TS type here to whichever type is used on the SDK-exported type in App.ts.

elle-j · 2023-10-16T16:33:00Z

integration-tests/tests/src/node/custom-fetch.ts

+  const HTTP_METHOD: Record<number, string> = {
+    [0]: "GET",
+    [1]: "POST",
+    [2]: "PUT",
+    [3]: "PATCH",
+    [4]: "DELETE",
+  };
+
+  const customFetch = async (request: Request) => {
+    called = true;
+    const { url, body, ...rest } = request;
+    if (typeof request.method == "number") {
+      rest.method = HTTP_METHOD[request.method];
+    }
+    const response = await fetch(url, rest);
+    return response;
+  };


I actually don't think you need the destructuring nor the typeof check in this case.

Since you're using node-fetch, the request.method will always be a string. Additionally, the destructuring can be removed because the 1st arg to fetch can be the Request itself.

The reason we were doing the typeof dance in our network.ts implementation is because the request could in our case also be a binding.Request (on which the method is an integer).

Suggested change

const HTTP_METHOD: Record<number, string> = {

[0]: "GET",

[1]: "POST",

[2]: "PUT",

[3]: "PATCH",

[4]: "DELETE",

};

const customFetch = async (request: Request) => {

called = true;

const { url, body, ...rest } = request;

if (typeof request.method == "number") {

rest.method = HTTP_METHOD[request.method];

}

const response = await fetch(url, rest);

return response;

};

const customFetch = (request: Request): Promise<Response> => {

called = true;

return fetch(request);

};

elle-j · 2023-10-16T16:40:44Z

CHANGELOG.md

+const HTTP_METHOD: Record<number, string> = {
+  [0]: "GET",
+  [1]: "POST",
+  [2]: "PUT",
+  [3]: "PATCH",
+  [4]: "DELETE",
+};
+
+const customFetch = async (request: Request) => {
+  const { url, body, ...rest } = request;
+  if (typeof request.method == "number") {
+    rest.method = HTTP_METHOD[request.method];
+  }
+  const response = await fetch(url, rest); // `fetch` is assumed to be compliant with the Fetch API specification
+  return response;
+};


I added a PR comment to integration-tests/tests/src/node/custom-fetch.ts explaining why I believe most of this can be simplified.

But also, for this CHANGELOG entry I think it could be good to show only what's necessary since users might have very different implementations of their custom fetch.

Suggested change

const HTTP_METHOD: Record<number, string> = {

[0]: "GET",

[1]: "POST",

[2]: "PUT",

[3]: "PATCH",

[4]: "DELETE",

};

const customFetch = async (request: Request) => {

const { url, body, ...rest } = request;

if (typeof request.method == "number") {

rest.method = HTTP_METHOD[request.method];

}

const response = await fetch(url, rest); // `fetch` is assumed to be compliant with the Fetch API specification

return response;

};

const customFetch = (request: Request): Promise<Response> => {

// `fetch` is assumed to be compliant with the Fetch API specification

return fetch(request);

};

elle-j · 2023-10-16T16:56:25Z

integration-tests/tests/src/node/custom-fetch.ts

+
+  it("custom fetch is called", async function (this: Mocha.Context & AppContext & RealmContext) {
+    let user;
+    called = false;


Since called is shared among all tests in this suite (if more tests were to be added), perhaps we could also set it to false in a before/afterEach to not be too dependent on the need to know that it should always be set to false first?

elle-j · 2023-10-16T16:58:11Z

integration-tests/tests/src/tests/sync/app.ts

+import { baseUrl, Response } from "../../utils/import-app";
 import { select } from "../../utils/select";
 import { buildAppConfig } from "../../utils/build-app-config";
+import { Fetch, Request } from "@realm/network-transport";
+import { fetch } from "../../utils/fetch";


I think the added imports (Response, Fetch, Request, fetch) can safely be removed. They don't seem to be used anywhere here.

elle-j · 2023-10-16T17:03:34Z

packages/realm/src/app-services/App.ts

+   * @since 12.3.0
+   * @experimental This API is experimental and may change or be removed.
+   */
+  fetchOverride?: unknown;


I think it should be similar to a specification, or if that is too specific for a custom fetch, perhaps just a function type such as () => Promise<unknown>. Also, I'd say we should try to not expose bindgen details to the public API.

elle-j · 2023-10-16T17:19:42Z

packages/realm/src/app-services/App.ts


+    if (fetchOverride) {
+      inject({ fetch: fetchOverride } as NetworkType);
+    }


If we decide to add a more explicit type than any or unknown to the fetchOverride in the app config (I think at least specifying that it should be a function could be good), then what do you think of adding an explicit check for undefined here? This way, we can let users know if they have provided an invalid falsy value in addition to an invalid truthy value by asserting that it's a function.

Suggested change

if (fetchOverride) {

inject({ fetch: fetchOverride } as NetworkType);

}

if (fetchOverride !== undefined) {

assert.function(fetchOverride, "fetchOverride");

inject({ fetch: fetchOverride } as NetworkType);

}

elle-j · 2023-10-16T17:22:14Z

packages/realm/src/platform/network.ts

@@ -60,5 +60,5 @@ export const network: NetworkType = {
 };

 export function inject(injected: NetworkType) {
-  Object.freeze(Object.assign(network, injected));
+  Object.assign(network, injected);


Did using .freeze() cause an issue here?

Unless this inject called multiple times, the freeze shouldn't be an issue.

Yeah - my suggestion (to @kneth) to remove the freeze was only if we choose to expose the inject function to end-users as a means of overriding the fetch implementation.

Gotcha 🙂

cla-bot bot added the cla: yes label Sep 29, 2023

kneth requested review from kraenhansen and elle-j September 29, 2023 14:00

github-actions bot assigned kneth Sep 29, 2023

kneth mentioned this pull request Sep 29, 2023

Starting from version 12 it is not possible to override the fetch API used by SDK #6159

Closed

kneth force-pushed the kneth/custom-network-transport branch from 493a83e to e721b8f Compare September 29, 2023 14:10

kneth marked this pull request as draft October 2, 2023 07:31

kraenhansen reviewed Oct 2, 2023

View reviewed changes

Override fetch for Altas App Services

9033d43

kneth force-pushed the kneth/custom-network-transport branch from e721b8f to 9033d43 Compare October 3, 2023 10:43

kneth added 3 commits October 3, 2023 16:21

Update changelog with an example

f93c725

Better types

b77bf38

Mark as experimental

ebb1879

kneth marked this pull request as ready for review October 3, 2023 14:46

kneth requested a review from takameyer October 3, 2023 14:46

takameyer reviewed Oct 16, 2023

View reviewed changes

elle-j reviewed Oct 16, 2023

View reviewed changes

kneth closed this May 23, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override fetch for Altas App Services #6165

Override fetch for Altas App Services #6165

kneth commented Sep 29, 2023

coveralls-official bot commented Sep 29, 2023 •

edited

Loading

kraenhansen Oct 2, 2023

elle-j Oct 16, 2023

kraenhansen Oct 2, 2023

kneth Oct 3, 2023

kraenhansen Oct 2, 2023

kneth Oct 3, 2023

takameyer Oct 17, 2023

takameyer Oct 16, 2023

takameyer Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023 •

edited

Loading

takameyer Oct 17, 2023

elle-j Oct 17, 2023

takameyer Oct 17, 2023 •

edited

Loading

elle-j Oct 18, 2023

kraenhansen Oct 21, 2023

takameyer Oct 23, 2023

kraenhansen Oct 23, 2023 •

edited

Loading

takameyer Oct 23, 2023 •

edited

Loading

elle-j left a comment

elle-j Oct 16, 2023

elle-j Oct 16, 2023

elle-j Oct 16, 2023

elle-j Oct 16, 2023

elle-j Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023

elle-j Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023

takameyer Oct 17, 2023

kraenhansen Oct 18, 2023 •

edited

Loading

elle-j Oct 18, 2023

		@@ -1 +1 @@
		{"REALM_ANONYMIZED_BUNDLE_ID":"tkgif/+3l1e9wStGJp2TOngAK3UcQ2u7OM8ZYJU5JYo="}

	* Expose `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))
	* Exposed `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))

Override fetch for Altas App Services #6165

Override fetch for Altas App Services #6165

Conversation

kneth commented Sep 29, 2023

What, How & Why?

☑️ ToDos

coveralls-official bot commented Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takameyer Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takameyer Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

takameyer Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls-official bot commented Sep 29, 2023 •

edited

Loading

takameyer Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023 •

edited

Loading

takameyer Oct 17, 2023 •

edited

Loading

kraenhansen Oct 23, 2023 •

edited

Loading

takameyer Oct 23, 2023 •

edited

Loading

elle-j Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023 •

edited

Loading

elle-j Oct 16, 2023 •

edited

Loading

kraenhansen Oct 18, 2023 •

edited

Loading