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

feat(http): Introduction of the fetch Backend for the HttpClient #50247

Closed
wants to merge 3 commits into from

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented May 11, 2023

This commit introduces a new HttpBackend implentation which makes requests using the fetch API

This feature is a developer preview and is opt-in.
It is enabled by setting the providers with provideHttpClient(withFetch()).

Currently the report progress is not implemented and using reportProgress: true will throw an error.

NB: The fetch API is experimental on Node but available without flags from Node 18 onwards.
NB2: The tests provided in the PR won't run, as fetch is not available on the node version provided by the bazel workspace (16.14)


Edit: pinning a answer I gave bellow on the purpose of this new backend:

Xhr is not supported natively on NodeJS an requires a polyfill. Currently Angular uses xhr2 for this. However, the polyfill is side-effectful and doesn't work in non-Node.js environments (such as Edge Workers). There are also others issues like #46930 (its doesn't support Gzip).

PR Type

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • No

Closes #27689

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label May 11, 2023
packages/common/http/src/fetch.ts Show resolved Hide resolved
packages/common/http/src/provider.ts Outdated Show resolved Hide resolved
packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Couple of initial comments (This is not a full review).

packages/common/http/src/fetch.ts Show resolved Hide resolved
packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
packages/common/http/src/provider.ts Outdated Show resolved Hide resolved
packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
@JeanMeche
Copy link
Member Author

Also, @alan-agius4 pointed out one DX issue. On projects with multi provideHttpClient, it's easy to forget a withFetch() call.

I'll look for a way to provide a global use of the FetchBackend

@JeanMeche JeanMeche force-pushed the feat/fetch-backend branch 2 times, most recently from 9698580 to 83a4bd4 Compare May 11, 2023 12:14
alan-agius4
alan-agius4 previously approved these changes May 11, 2023
Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Thanks @JeanMeche for this.

packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
packages/common/http/test/fetch_spec.ts Outdated Show resolved Hide resolved
packages/common/http/test/fetch_spec.ts Outdated Show resolved Hide resolved
packages/common/http/test/fetch_spec.ts Outdated Show resolved Hide resolved
packages/common/http/test/fetch_spec.ts Outdated Show resolved Hide resolved
packages/common/http/src/fetch.ts Outdated Show resolved Hide resolved
packages/common/http/src/fetch.ts Show resolved Hide resolved
@alan-agius4 alan-agius4 self-requested a review May 11, 2023 12:45
@alan-agius4 alan-agius4 dismissed their stale review May 11, 2023 12:46

Approved by mistake.

@alan-agius4 alan-agius4 removed their request for review May 11, 2023 12:46
@HyperLife1119
Copy link
Contributor

The HttpRequest#reportProgress option is not available for the Fetch API, maybe we need to check for it and throw an exception?

@alan-agius4
Copy link
Contributor

@HyperLife1119, we can explore/add progress in a follow up.

@alan-agius4
Copy link
Contributor

By the way the commit scope should be http instead of common.

@JeanMeche JeanMeche force-pushed the feat/fetch-backend branch 2 times, most recently from 85aeb62 to a01026e Compare May 11, 2023 17:35
@JeanMeche JeanMeche changed the title feat(common): Introduction of the fetch Backend for the HttpClient feat(http): Introduction of the fetch Backend for the HttpClient May 11, 2023
@alan-agius4 alan-agius4 added area: common/http target: minor This PR is targeted for the next minor release labels May 11, 2023
@ngbot ngbot bot added this to the Backlog milestone May 11, 2023
@JeanMeche JeanMeche force-pushed the feat/fetch-backend branch 3 times, most recently from 3c4064b to a540d44 Compare May 11, 2023 18:04
@JeanMeche JeanMeche marked this pull request as ready for review May 11, 2023 18:04
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jun 6, 2023
@AndrewKushnir
Copy link
Contributor

The node version commit should be before the HTTP commit, so that the test doesn't fail on that commit.

@JeanMeche could you please also merge all fixup commits into the main commit while reordering them based on the feedback above? Thank you.

alan-agius4 and others added 2 commits June 7, 2023 00:25
This commits adds configures `//packages/common/http/test` to use Node.js 18 toolchain which is needed to test the fetch implementation which use Node.js 18 APIs.

We also disable RBE for this target as it doesn't work with Node.js 18 right now. See angular/dev-infra#1017
This commit introduces a new `HttpBackend` implentation which makes requests using the fetch API

This feature is a developer preview and is opt-in.
It is enabled by setting the providers with `provideHttpClient(withFetch())`.

NB: The fetch API is experimental on Node but available without flags from Node 18 onwards.
@AndrewKushnir AndrewKushnir force-pushed the feat/fetch-backend branch 3 times, most recently from fcac36f to a761c27 Compare June 8, 2023 00:36
@AndrewKushnir AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Jun 8, 2023
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir
Copy link
Contributor

TGP.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jun 8, 2023
@AndrewKushnir
Copy link
Contributor

Caretaker note: TGP looks good, current set of approvals should be sufficient to proceed with the merge.

// cc @alxhub

@alxhub
Copy link
Member

alxhub commented Jun 8, 2023

This PR was merged into the repository by commit 85c5427.

@alxhub alxhub closed this in 37d3664 Jun 8, 2023
alxhub pushed a commit that referenced this pull request Jun 8, 2023
…50247)

This commit introduces a new `HttpBackend` implentation which makes requests using the fetch API

This feature is a developer preview and is opt-in.
It is enabled by setting the providers with `provideHttpClient(withFetch())`.

NB: The fetch API is experimental on Node but available without flags from Node 18 onwards.

PR Close #50247
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 15, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fanimations/16.0.4/16.1.1) |
| [@angular/common](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcommon/16.0.4/16.1.1) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/16.0.4/16.1.1) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/16.0.4/16.1.1) |
| [@angular/core](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcore/16.0.4/16.1.1) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fforms/16.0.4/16.1.1) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/16.0.4/16.1.1) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/16.0.4/16.1.1) |
| [zone.js](https://github.com/angular/angular) ([changelog](https://github.com/angular/angular/blob/master/packages/zone.js/CHANGELOG.md)) | dependencies | patch | [`0.13.0` -> `0.13.1`](https://renovatebot.com/diffs/npm/zone.js/0.13.0/0.13.1) |

---

### Release Notes

<details>
<summary>angular/angular (@&#8203;angular/animations)</summary>

### [`v16.1.1`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1611-2023-06-14)

[Compare Source](angular/angular@16.1.0...16.1.1)

##### compiler-cli

| Commit | Type | Description |
| -- | -- | -- |
| [71360b3a3e](angular/angular@71360b3) | fix | libraries compiled with v16.1+ breaking with Angular framework v16.0.x ([#&#8203;50715](angular/angular#50715)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [d9bed48eb5](angular/angular@d9bed48) | fix | extend toSignal to accept any Subscribable ([#&#8203;50162](angular/angular#50162)) |

##### migrations

| Commit | Type | Description |
| -- | -- | -- |
| [5e1d8444ae](angular/angular@5e1d844) | fix | Prevent a component from importing itself. ([#&#8203;50554](angular/angular#50554)) |

<!-- CHANGELOG SPLIT MARKER -->

### [`v16.1.0`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1610-2023-06-13)

[Compare Source](angular/angular@16.0.6...16.1.0)

##### compiler

| Commit | Type | Description |
| -- | -- | -- |
| [4e663297c5](angular/angular@4e66329) | fix | error when reading compiled input transforms metadata in JIT mode ([#&#8203;50600](angular/angular#50600)) |
| [721bc72649](angular/angular@721bc72) | fix | resolve deprecation warning with TypeScript 5.1 ([#&#8203;50460](angular/angular#50460)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [68017d4e75](angular/angular@68017d4) | feat | add ability to transform input values ([#&#8203;50420](angular/angular#50420)) |
| [69dadd2502](angular/angular@69dadd2) | feat | support TypeScript 5.1 ([#&#8203;50156](angular/angular#50156)) |
| [c0ebe34cbd](angular/angular@c0ebe34) | fix | add additional component metadata to component ID generation ([#&#8203;50336](angular/angular#50336)) |

##### http

| Commit | Type | Description |
| -- | -- | -- |
| [85c5427582](angular/angular@85c5427) | feat | Introduction of the `fetch` Backend for the `HttpClient` ([#&#8203;50247](angular/angular#50247)) |

<!-- CHANGELOG SPLIT MARKER -->

### [`v16.0.6`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1606-2023-06-13)

[Compare Source](angular/angular@16.0.5...16.0.6)

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [05ac0868c9](angular/angular@05ac086) | fix | avoid duplicated content during hydration while processing a component with i18n ([#&#8203;50644](angular/angular#50644)) |

<!-- CHANGELOG SPLIT MARKER -->

### [`v16.0.5`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1605-2023-06-08)

[Compare Source](angular/angular@16.0.4...16.0.5)

##### compiler

| Commit | Type | Description |
| -- | -- | -- |
| [703b8fcac1](angular/angular@703b8fc) | fix | do not remove comments in component styles ([#&#8203;50346](angular/angular#50346)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [2b6da93e19](angular/angular@2b6da93) | fix | incorrectly throwing error for self-referencing component ([#&#8203;50559](angular/angular#50559)) |
| [c992109d6c](angular/angular@c992109) | fix | wait for HTTP in `ngOnInit` correctly before server render ([#&#8203;50573](angular/angular#50573)) |

##### platform-server

| Commit | Type | Description |
| -- | -- | -- |
| [c0d4086c6e](angular/angular@c0d4086) | fix | surface errors during rendering ([#&#8203;50587](angular/angular#50587)) |

<!-- CHANGELOG SPLIT MARKER -->

</details>

<details>
<summary>angular/angular (zone.js)</summary>

### [`v0.13.1`](https://github.com/angular/angular/blob/HEAD/packages/zone.js/CHANGELOG.md#v0131-httpsgithubcomangularangularcomparezonejs-0130zonejs-v0131-2023-06-09)

[Compare Source](angular/angular@zone.js-0.13.0...zone.js-0.13.1)

##### Bug Fixes

-   **zone.js:** enable monkey patching of the `queueMicrotask()` API in node.js ([#&#8203;50467](angular/angular#50467)) ([381cb98](angular/angular@381cb98))
-   **zone.js:** enable monkey patching of the `queueMicrotask()` API in node.js ([#&#8203;50530](angular/angular#50530)) ([7837f71](angular/angular@7837f71))
-   **zone.js:** patch entire promise in node ([#&#8203;50552](angular/angular#50552)) ([cb31dbc](angular/angular@cb31dbc)), closes [#&#8203;50513](angular/angular#50513) [#&#8203;50457](angular/angular#50457) [#&#8203;50414](angular/angular#50414) [#&#8203;49930](angular/angular#49930)
-   **zone.js:** revert Mocha it.skip, describe.skip method patch ([#&#8203;49329](angular/angular#49329)) ([5a2b622](angular/angular@5a2b622))

##### Features

-   **zone.js:** jest 29 should ignore uncaught error console log ([#&#8203;49325](angular/angular#49325)) ([bc412fd](angular/angular@bc412fd)), closes [#&#8203;49110](angular/angular#49110)

##### Reverts

-   Revert "fix(zone.js): enable monkey patching of the `queueMicrotask()` API in node.js ([#&#8203;50467](angular/angular#50467))" ([#&#8203;50529](angular/angular#50529)) ([7567348](angular/angular@7567348)), closes [#&#8203;50467](angular/angular#50467) [#&#8203;50529](angular/angular#50529) [#&#8203;50529](angular/angular#50529)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTUuMiIsInVwZGF0ZWRJblZlciI6IjM1LjExNy4yIiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1927
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 9, 2023
@JeanMeche JeanMeche deleted the feat/fetch-backend branch July 12, 2023 08:08
thomasturrell pushed a commit to thomasturrell/angular that referenced this pull request Aug 29, 2023
This commits adds configures `//packages/common/http/test` to use Node.js 18 toolchain which is needed to test the fetch implementation which use Node.js 18 APIs.

We also disable RBE for this target as it doesn't work with Node.js 18 right now. See angular/dev-infra#1017

PR Close angular#50247
thomasturrell pushed a commit to thomasturrell/angular that referenced this pull request Aug 29, 2023
…ngular#50247)

This commit introduces a new `HttpBackend` implentation which makes requests using the fetch API

This feature is a developer preview and is opt-in.
It is enabled by setting the providers with `provideHttpClient(withFetch())`.

NB: The fetch API is experimental on Node but available without flags from Node 18 onwards.

PR Close angular#50247
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
This commits adds configures `//packages/common/http/test` to use Node.js 18 toolchain which is needed to test the fetch implementation which use Node.js 18 APIs.

We also disable RBE for this target as it doesn't work with Node.js 18 right now. See angular/dev-infra#1017

PR Close angular#50247
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…ngular#50247)

This commit introduces a new `HttpBackend` implentation which makes requests using the fetch API

This feature is a developer preview and is opt-in.
It is enabled by setting the providers with `provideHttpClient(withFetch())`.

NB: The fetch API is experimental on Node but available without flags from Node 18 onwards.

PR Close angular#50247
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project area: common/http detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] [Fetch API] Support for JavaScript fetch API