-
Notifications
You must be signed in to change notification settings - Fork 585
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
Override fetch for Altas App Services #6165
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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)) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
```js | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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; | ||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+10
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a PR comment to 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
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
let app = new Realm.App({ id: "my-app-id", fetchOverride: customFetch }); | ||||||||||||||||||||||||||||||||||||||||||
``` | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
### Fixed | ||||||||||||||||||||||||||||||||||||||||||
* Outside migration functions, it is not possible to change the value of a primary key. ([#6161](https://github.com/realm/realm-js/issues/6161), since v12.0.0) | ||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -32,6 +32,7 @@ export type AppConfigurationRelaxed = { | |||||
timeout?: number; | ||||||
multiplexSessions?: boolean; | ||||||
baseFilePath?: string; | ||||||
fetchOverride?: any; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My immediate reaction is that the "override" part of this name is redundant as all of these options override default values.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Additionally, would also be good to match the TS type here to whichever type is used on the SDK-exported type in |
||||||
}; | ||||||
|
||||||
export function importAppBefore(config: AppConfig | { config: AppConfig }, sdkConfig?: AppConfigurationRelaxed): void { | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,66 @@ | ||||||||||||||||||||||||||||||||||||||||||||
//////////////////////////////////////////////////////////////////////////// | ||||||||||||||||||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||||||||||||||||
// Copyright 2022 Realm Inc. | ||||||||||||||||||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||||||||||||||||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||||||||||||||||||||||
// you may not use this file except in compliance with the License. | ||||||||||||||||||||||||||||||||||||||||||||
// You may obtain a copy of the License at | ||||||||||||||||||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||||||||||||||||
// http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||||||||||||||||
// Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||||||||||||||||||||||||||
// distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||||||||||||||||||||||||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||||||||||||||||||||||||||
// See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||||||||||||||||||
// limitations under the License. | ||||||||||||||||||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||||||||||||||||
//////////////////////////////////////////////////////////////////////////// | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
import { expect } from "chai"; | ||||||||||||||||||||||||||||||||||||||||||||
import { importAppBefore } from "../hooks"; | ||||||||||||||||||||||||||||||||||||||||||||
import { buildAppConfig } from "../utils/build-app-config"; | ||||||||||||||||||||||||||||||||||||||||||||
import fetch, { Request } from "node-fetch"; | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
import Realm from "realm"; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
describe("custom fetch", () => { | ||||||||||||||||||||||||||||||||||||||||||||
let called = false; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const HTTP_METHOD: Record<number, string> = { | ||||||||||||||||||||||||||||||||||||||||||||
[0]: "GET", | ||||||||||||||||||||||||||||||||||||||||||||
[1]: "POST", | ||||||||||||||||||||||||||||||||||||||||||||
[2]: "PUT", | ||||||||||||||||||||||||||||||||||||||||||||
[3]: "PATCH", | ||||||||||||||||||||||||||||||||||||||||||||
[4]: "DELETE", | ||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+28
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it'll make more sense to export this somehow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, but I have a feeling that we might leak too many implementation details. |
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
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; | ||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+36
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alternative could be to implement this as a wrapping of the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this solution is good, otherwise we would also have to provide a mechanism to override the default
Comment on lines
+28
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually don't think you need the destructuring nor the Since you're using The reason we were doing the
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
importAppBefore(buildAppConfig("with-anon").anonAuth(), { fetchOverride: customFetch }); | ||||||||||||||||||||||||||||||||||||||||||||
afterEach(() => { | ||||||||||||||||||||||||||||||||||||||||||||
Realm.clearTestState(); | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
it("custom fetch is called", async function (this: Mocha.Context & AppContext & RealmContext) { | ||||||||||||||||||||||||||||||||||||||||||||
let user; | ||||||||||||||||||||||||||||||||||||||||||||
called = false; | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since |
||||||||||||||||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||||||||||||||||
expect(this.app).instanceOf(Realm.App); | ||||||||||||||||||||||||||||||||||||||||||||
const credentials = Realm.Credentials.anonymous(); | ||||||||||||||||||||||||||||||||||||||||||||
user = await this.app.logIn(credentials); | ||||||||||||||||||||||||||||||||||||||||||||
expect(called).equals(true); | ||||||||||||||||||||||||||||||||||||||||||||
expect(user).instanceOf(Realm.User); | ||||||||||||||||||||||||||||||||||||||||||||
expect(user.deviceId).to.not.be.null; | ||||||||||||||||||||||||||||||||||||||||||||
expect(user.providerType).equals("anon-user"); | ||||||||||||||||||||||||||||||||||||||||||||
} finally { | ||||||||||||||||||||||||||||||||||||||||||||
await user?.logOut(); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,5 +20,6 @@ | |
|
||
import "./analytics"; | ||
import "./clean-exit"; | ||
import "./custom-fetch"; | ||
import "./path"; | ||
import "./sync-proxy"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,9 +21,11 @@ import Realm, { AppConfiguration, BSON, MetadataMode } from "realm"; | |
|
||
import { importAppBefore } from "../../hooks"; | ||
import { generatePartition } from "../../utils/generators"; | ||
import { baseUrl } from "../../utils/import-app"; | ||
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"; | ||
Comment on lines
+24
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the added imports ( |
||
|
||
const TestObjectSchema: Realm.ObjectSchema = { | ||
primaryKey: "_id", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
{"REALM_ANONYMIZED_BUNDLE_ID":"tkgif/+3l1e9wStGJp2TOngAK3UcQ2u7OM8ZYJU5JYo="} | ||
{"REALM_ANONYMIZED_BUNDLE_ID":"1RmJBlqbKuzyRiPm4AsdIIxe8xlRUntGcGFEwUnUh6A="} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -16,6 +16,7 @@ | |||||||||||||||||
// | ||||||||||||||||||
//////////////////////////////////////////////////////////////////////////// | ||||||||||||||||||
|
||||||||||||||||||
import { NetworkType, inject } from "src/platform/network"; | ||||||||||||||||||
import { | ||||||||||||||||||
AnyUser, | ||||||||||||||||||
Credentials, | ||||||||||||||||||
|
@@ -140,6 +141,13 @@ export type AppConfiguration = { | |||||||||||||||||
* @since 12.2.0 | ||||||||||||||||||
*/ | ||||||||||||||||||
metadata?: Metadata; | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Specify a custom `fetch` implementation. | ||||||||||||||||||
* @since 12.3.0 | ||||||||||||||||||
* @experimental This API is experimental and may change or be removed. | ||||||||||||||||||
*/ | ||||||||||||||||||
fetchOverride?: unknown; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we specify this type to match the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will there be unintended side effects if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you referring to our But, safest would probably be to ask the user of their intended use (arg and return type) and if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The user in this case just wants to replace 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 From this observation, perhaps @kraenhansen is correct in that we should directly override the It's probably worth while doing a manual test in React Native, to see if such an override could cause significant issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My immediate feeling is that I like the declarative API of keeping this a configuration of the I like ☝️ over this more procedural There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intention of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @takameyer I don't see how that'll work. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
|
@@ -261,7 +269,8 @@ export class App< | |||||||||||||||||
constructor(configOrId: AppConfiguration | string) { | ||||||||||||||||||
const config: AppConfiguration = typeof configOrId === "string" ? { id: configOrId } : configOrId; | ||||||||||||||||||
assert.object(config, "config"); | ||||||||||||||||||
const { id, baseUrl, app, timeout, multiplexSessions = true, baseFilePath, metadata } = config; | ||||||||||||||||||
const { id, baseUrl, app, timeout, multiplexSessions = true, baseFilePath, metadata, fetchOverride } = config; | ||||||||||||||||||
|
||||||||||||||||||
assert.string(id, "id"); | ||||||||||||||||||
if (timeout !== undefined) { | ||||||||||||||||||
assert.number(timeout, "timeout"); | ||||||||||||||||||
|
@@ -277,6 +286,10 @@ export class App< | |||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if (fetchOverride) { | ||||||||||||||||||
inject({ fetch: fetchOverride } as NetworkType); | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
288
to
+291
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we decide to add a more explicit type than
Suggested change
|
||||||||||||||||||
|
||||||||||||||||||
fs.ensureDirectoryForFile(fs.joinPaths(baseFilePath || fs.getDefaultDirectoryPath(), "mongodb-realm")); | ||||||||||||||||||
// TODO: This used getSharedApp in the legacy SDK, but it's failing AppTests | ||||||||||||||||||
this.internal = binding.App.getUncachedApp( | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ export type { FetchHeaders, Request }; | |
const debug = extendDebug("network"); | ||
const transport = new DefaultNetworkTransport(); | ||
|
||
type NetworkType = { | ||
export type NetworkType = { | ||
fetch(request: Request): Promise<FetchResponse>; | ||
fetch(request: binding.Request): Promise<FetchResponse>; | ||
}; | ||
|
@@ -60,5 +60,5 @@ export const network: NetworkType = { | |
}; | ||
|
||
export function inject(injected: NetworkType) { | ||
Object.freeze(Object.assign(network, injected)); | ||
Object.assign(network, injected); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah - my suggestion (to @kneth) to remove the freeze was only if we choose to expose the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha 🙂 |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should say
AppConfiguration.fetchOverride
rather thanRealm.App.Configuration.fetchOverride
due to how users currently can import it. Atm it seems like they can only import it via{ AppConfiguration }
.