Remove `node-fetch` dependency from SDK, introduce `@realm/fetch` and allow passing `fetch` through `AppConfiguration` #6401

kraenhansen · 2024-01-22T11:05:33Z

What, How & Why?

This fixes #6393 and makes #6367 obsolete as it removes the node-fetch dependency, in favour of depending on the fetch provided by Node.js at v18 and above.

It also replaces @realm/network-transport with a private (not to be published on NPM) @realm/fetch package. The new package exports types reflecting the lowest common API for fetch across our supported platforms and adds platform specific polyfills to the fetch API. This will help ease exposing hooks for end-users to provide their own fetch implementation (ensuring it's easier to support a use-case like that outlined in #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 · 2024-01-22T11:43:45Z

coverage: 85.346% (-0.5%) from 85.833%
when pulling 8af55d2 on kh/removing-node-fetch
into 4408591 on main.

kraenhansen · 2024-01-23T17:10:32Z

Can we expose fetch on AppConfiguration as we do for Realm Web?

It turned out to be pretty simple 👍 6f432ab .. I've added two tests, one with an trivial throwing implementation and another passing node-fetch (which ironically was the reason for PR in the first place 🤦)

kraenhansen · 2024-01-24T06:07:28Z

@takameyer I've requested review since I've added functionality since your initial review.

elle-j

Great replacement here, interesting walkthru and PR!

elle-j · 2024-01-24T08:38:11Z

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

+  it("is supported", async function (this: AppContext) {
+    const app = new Realm.App({ id: this.app.id, baseUrl, fetch: nodeFetch });
+    const user = await app.logIn(Realm.Credentials.anonymous());
+    expect(typeof user.id).equals("string");


Assuming the test interprets a successful login as "passing node-fetch works" and a failed login as the opposite, I'm thinking that testing for a non-null user may be more appropriate here (if at all needed since the login will throw if it fails). For successful logins, the id will always be a string as far as I know 🙂

Suggested change

expect(typeof user.id).equals("string");

expect(app.currentUser).not.to.be.null;

I could probably just await the login, instead of actually expecting anything of the result 🤔

The test doesn't actually verify that the fetch provided will be used, I added another (more general) test for that. The main purpose of this is actually that I wanted to test that the type for the fetch option would accept the node-fetch export.

Yup I agree, the expect is likely not necessary for the test 👍

packages/fetch/src/types.ts

packages/realm-app-importer/src/AdminApiClient.ts

packages/realm-web/src/App.ts

packages/realm-web/src/Fetcher.ts

packages/fetch/src/types.ts

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

Co-authored-by: LJ <[email protected]> Co-authored-by: Andrew Meyer <[email protected]>

takameyer

Ship it!

… allow passing `fetch` through `AppConfiguration` (realm#6401) * Removed node-fetch and abort-controller from dependencies * Hoisted dependencies on typescript and @types/node * Started implementing a truely isometric fetch package * Removed the path alias for types * WIP * Fixed Headers types * Adding AbortSignal * Removing types project as reference in root tsconfig.json * assserting AbortSignal types and patching timeout static * Adding AbortController * Exporting Headers and returning Response * ReadableStream WIP * WIP * Deleted @realm/network-transport * Adding textStreaming config to React Native fetch requests. * Adding engines and type param to @realm/fetch * Removing network-transport from mono repo * Using @realm/fetch from "realm" * Using @realm/fetch from "app-importer" * Using @realm/fetch from "realm-web" * WIP * Ensure @realm/fetch is in the bundle of realm-web * Ensure @realm/fetch is in the bundle of realm * Use a utility to wrap response.body in an async iterator * Adding a version to @realm/fetch * Adding engines to realm-web package.json * Fixing react-native fetch implementation * Adding comments to explain the weird emits * Adding a "type-check" script to app-importer * Adding a top-level "react-native" in @realm/fetch's package.json * Prodiving a CJS + ESM node bundle * Renamed fetch script to "build" (since it doesn't bundle) * Bind fetch on node too (as this is loaded on Realm * Debugging url and init separately * Simplified types for ReactNative request init * Implemented RN polyfill for AbortSignal.timeout * Fixed setting statusText instead of status * Using AbortSignal.timeout only if timeout is positive * Adding an AnyFetch type * Enable passing "fetch" through app configuration * Passing baseUrl in the node-fetch test * Providing a fallback for Symbol.asyncIterator if transpiled using Babel * Apply suggestions from code review Co-authored-by: LJ <[email protected]> Co-authored-by: Andrew Meyer <[email protected]> * Inlining configuration.storage * Making cancel() return Promise<void> * Adding ArrayBuffer | TypedArray to RequestBody * Bumping lib of realm-web to es2022 --------- Co-authored-by: LJ <[email protected]> Co-authored-by: Andrew Meyer <[email protected]>

kraenhansen self-assigned this Jan 22, 2024

kraenhansen added 24 commits January 22, 2024 12:08

Removed node-fetch and abort-controller from dependencies

0f40fed

Hoisted dependencies on typescript and @types/node

891bbe1

Started implementing a truely isometric fetch package

6de5563

Removed the path alias for types

1b653c3

WIP

63758fa

Fixed Headers types

f104f44

Adding AbortSignal

9d99127

Removing types project as reference in root tsconfig.json

f58bc74

assserting AbortSignal types and patching timeout static

76460cc

Adding AbortController

8e57b33

Exporting Headers and returning Response

13f6364

ReadableStream WIP

3b4243e

WIP

6711cc7

Deleted @realm/network-transport

207640a

Adding textStreaming config to React Native fetch requests.

75da88b

Adding engines and type param to @realm/fetch

f6e2b13

Removing network-transport from mono repo

6b8a5f6

Using @realm/fetch from "realm"

624a833

Using @realm/fetch from "app-importer"

4dcba93

Using @realm/fetch from "realm-web"

eded4fa

WIP

5a70b34

Ensure @realm/fetch is in the bundle of realm-web

2015585

Ensure @realm/fetch is in the bundle of realm

e4535c6

Use a utility to wrap response.body in an async iterator

0c631da

kraenhansen force-pushed the kh/removing-node-fetch branch from b429752 to 0c631da Compare January 22, 2024 11:10

Adding a version to @realm/fetch

fd1fca4

kraenhansen added 2 commits January 22, 2024 12:54

Adding engines to realm-web package.json

0bdfa96

Fixing react-native fetch implementation

4350d2d

kraenhansen added 7 commits January 23, 2024 17:05

Debugging url and init separately

2eb6242

Simplified types for ReactNative request init

6e9a314

Implemented RN polyfill for AbortSignal.timeout

685bb48

Fixed setting statusText instead of status

e689429

Using AbortSignal.timeout only if timeout is positive

b26a475

Adding an AnyFetch type

ede8128

Enable passing "fetch" through app configuration

6f432ab

kraenhansen changed the title ~~Remove node-fetch dependency & introduce @realm/fetch~~ Remove node-fetch dependency from SDK, introduce @realm/fetch and allow passing fetch through AppConfiguration Jan 23, 2024

kraenhansen added 2 commits January 23, 2024 21:46

Passing baseUrl in the node-fetch test

4cc0c19

Providing a fallback for Symbol.asyncIterator if transpiled using Babel

0bff535

kraenhansen requested a review from takameyer January 24, 2024 05:54

elle-j approved these changes Jan 24, 2024

View reviewed changes

kraenhansen commented Jan 24, 2024

View reviewed changes

packages/fetch/src/types.ts Show resolved Hide resolved

kraenhansen commented Jan 24, 2024

View reviewed changes

integration-tests/tests/src/node/node-fetch.ts Outdated Show resolved Hide resolved

kraenhansen and others added 3 commits January 24, 2024 12:17

Apply suggestions from code review

7a46fc4

Co-authored-by: LJ <[email protected]> Co-authored-by: Andrew Meyer <[email protected]>

Inlining configuration.storage

6adbc52

Making cancel() return Promise<void>

538ae38

takameyer approved these changes Jan 24, 2024

View reviewed changes

kraenhansen requested a review from kneth January 24, 2024 14:35

rossicler-hostalky mentioned this pull request Jan 24, 2024

Process keep hanging after electron app is closed (Windows) #6357

Closed

Adding ArrayBuffer | TypedArray to RequestBody

eb5d38c

cla-bot bot added the cla: yes label Jan 24, 2024

Bumping lib of realm-web to es2022

8af55d2

kneth approved these changes Jan 25, 2024

View reviewed changes

kraenhansen merged commit e7e8d9f into main Jan 25, 2024
32 of 34 checks passed

kraenhansen deleted the kh/removing-node-fetch branch January 25, 2024 08:49

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `node-fetch` dependency from SDK, introduce `@realm/fetch` and allow passing `fetch` through `AppConfiguration` #6401

Remove `node-fetch` dependency from SDK, introduce `@realm/fetch` and allow passing `fetch` through `AppConfiguration` #6401

kraenhansen commented Jan 22, 2024

coveralls-official bot commented Jan 22, 2024 •

edited

Loading

kraenhansen commented Jan 23, 2024 •

edited

Loading

kraenhansen commented Jan 24, 2024

elle-j left a comment

elle-j Jan 24, 2024 •

edited

Loading

kraenhansen Jan 24, 2024

elle-j Jan 24, 2024

takameyer left a comment

	expect(typeof user.id).equals("string");
	expect(app.currentUser).not.to.be.null;

Remove node-fetch dependency from SDK, introduce @realm/fetch and allow passing fetch through AppConfiguration #6401

Remove node-fetch dependency from SDK, introduce @realm/fetch and allow passing fetch through AppConfiguration #6401

Conversation

kraenhansen commented Jan 22, 2024

What, How & Why?

☑️ ToDos

coveralls-official bot commented Jan 22, 2024 • edited Loading

kraenhansen commented Jan 23, 2024 • edited Loading

kraenhansen commented Jan 24, 2024

elle-j left a comment

Choose a reason for hiding this comment

elle-j Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

kraenhansen Jan 24, 2024

Choose a reason for hiding this comment

elle-j Jan 24, 2024

Choose a reason for hiding this comment

takameyer left a comment

Choose a reason for hiding this comment

Remove `node-fetch` dependency from SDK, introduce `@realm/fetch` and allow passing `fetch` through `AppConfiguration` #6401

Remove `node-fetch` dependency from SDK, introduce `@realm/fetch` and allow passing `fetch` through `AppConfiguration` #6401

coveralls-official bot commented Jan 22, 2024 •

edited

Loading

kraenhansen commented Jan 23, 2024 •

edited

Loading

elle-j Jan 24, 2024 •

edited

Loading