fix(common): stricter type for http methods #44328

cexbrayat · 2021-12-01T14:57:43Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

It's currently possible to use any string as the HTTP method.
As discussed with Pete and @AndrewKushnir it could be interesting to have a more strictly typed API,
and only allow valid HTTP methods (both in lowercase and uppercase to preserve the existing behavior)

What is the new behavior?

This commit tightens the API to only accept one of the possible HTTP verb, either in lowercase or in uppercase (to preserve the existing behavior).

Does this PR introduce a breaking change?

Yes
No

The HTTP method must now be one of 'GET'|'HEAD'|'POST'|'PUT'|'DELETE'|'CONNECT'|'OPTIONS'|'TRACE'|'PATCH'|'JSONP' (or one of these in lowercase) instead of a string. This helps developers to catch possible typos like http.request('POTS', ...), but can result in compilation errors when updating if you have such typos in your code. In the rare cases where developers used a temporary string variable to store the method, the type of this variable must be updated to be a string literal, for example const method = 'GET' as const.

Other information

cexbrayat · 2021-12-01T14:58:53Z

packages/common/http/public_api.ts

I exposed the type to reuse it in common/http/testing for RequestMatch, but maybe there is a better way?

petebacondarwin · 2021-12-01T17:52:58Z

packages/common/http/src/client.ts

Rather than having this union everywhere, how about making the HttpMethod type cover both upper and lower case?

I agree with @petebacondarwin. This would really simplify things.

I went back and forth on how to declare this, I've changed it to remove the union type 👍

jessicajaniuk

This looks very breaking to me as this service is used by everyone and would require everyone to change to a type. We'd definitely need a migration.

jessicajaniuk · 2021-12-01T18:05:47Z

packages/common/http/src/client.ts

I agree with @petebacondarwin. This would really simplify things.

The current API accepts `string` for the HTTP method. This commit tightens the API to only accept one of the possible HTTP verb, either in lowercase or in uppercase (to preserve the existing behavior). BREAKING CHANGE: The HTTP method must now be one of `'GET'|'HEAD'|'POST'|'PUT'|'DELETE'|'CONNECT'|'OPTIONS'|'TRACE'|'PATCH'|'JSONP'` (or one of these in lowercase) instead of a `string`. This helps developers to catch possible typos like `http.request('POTS', ...)`, but can result in compilation errors when updating if you have such typos in your code. In the rare cases where developers used a temporary string variable to store the method, the type of this variable must be updated to be a string literal, for example `const method = 'GET' as const`.

cexbrayat · 2021-12-01T18:57:33Z

@jessicajaniuk yeah, we were wondering on Slack with Pete and Andrew how much that would be breaking: if you or another Googler can run a presubmit, that would be great! It might not be that bad as most people are probably not using request that much and, if they do, probably call it with a string literal already. Same for tests and RequestMatch. But let's see!

jessicajaniuk · 2021-12-01T20:32:03Z

@cexbrayat I ran a presubmit and it's definitely breaking. The presubmit failed pretty significantly.

jessicajaniuk · 2021-12-01T20:33:25Z

goldens/public-api/common/http/http.md

    }): Observable<T>;
    request<R>(req: HttpRequest<any>): Observable<HttpEvent<R>>;
-    request(method: string, url: string, options: {
+    request(method: HttpMethod, url: string, options: {


To avoid this being breaking, this should really be string|HttpMethod anywhere HttpMethod would be accepted. This would ensure everything currently still works, but also can be typed.

jessicajaniuk · 2021-12-01T20:35:48Z

goldens/public-api/common/http/http.md

        withCredentials?: boolean;
    });
-    constructor(method: string, url: string, body: T | null, init?: {
+    constructor(method: HttpMethod, url: string, body: T | null, init?: {


I suggest changing method to be string|HttpMethod and then attempting to coerce the string version to the proper HttpMethod option. If it can't be coerced, then we throw. That would be the least breaky and would still allow for strong typing.

That's the same as being just string which kind of defeats the point (types arithmetic)

jessicajaniuk · 2021-12-01T20:36:56Z

packages/common/http/src/request.ts

   * The outgoing HTTP request method.
   */
-  readonly method: string;
+  readonly method: HttpMethod;


Same thing here with string|HttpMethod

AndrewKushnir · 2021-12-02T19:22:29Z

@cexbrayat just wanted to provide a quick update after team's discussion:

We'd need to run a wider set of tests in Google's codebase to better understand the impact. We'll keep you updated.
Could you please include some information on the source of the HTTP method list (how can we make sure that it's complete)? We'd need to include this information into the commit message.
Having strict list of HTTP method makes it difficult to use custom ones. If I understand correctly, this is not very common (and in some cases not considered a good practice), but we'd need to do some research to make sure we are not restricting valid use-cases. Could you please try to find quotes from HTTP specs on custom methods?
It'd be great to check how this change is aligned with other popular HTTP libs. Could you please help with this item? Here are some quick examples:
- Axios typings (list of HTTP methods)
- TypeScript fetch method typings + the Request interface (just typed as string)

Once we get more info based on the items above, we can make a final decision on how to proceed.

cexbrayat · 2021-12-03T08:59:24Z

@AndrewKushnir Sure

OK let's see
I used the HTTP methods referenced in RFC 7231, + PATCH (RFC 5789) and JSONP.
The RFC also mentions:

 Additional methods, outside the scope of this specification, have
   been standardized for use in HTTP.  All such methods ought to be
   registered within the "Hypertext Transfer Protocol (HTTP) Method
   Registry" maintained by IANA, as defined in Section 8.1.

The IANA registry defines a lot of other methods with their own RFC, see https://www.iana.org/assignments/http-methods/http-methods.xhtml
3. I don't think something forbids creating a custom HTTP method in the existing RFCs
4. TS lib.dom uses string (as the backing Web IDL uses string as well).
Axios does offer a typed version of request, with all the methods of this PR plusLINK, UNLINK and PURGE (this one is not in the IANA registry). Superagent is using string (in its DT typings).

If this is too breaking, we can definitely settle for a union type HttpMethod|string as @jessicajaniuk suggested: this would still improve DX.

cexbrayat · 2021-12-03T09:41:03Z

@AndrewKushnir @jessicajaniuk I opened #44363 as an alternative based on a union type, if you want to check how less breaky it is.

AndrewKushnir · 2021-12-07T01:12:03Z

Just a quick update on:

We'd need to run a wider set of tests in Google's codebase to better understand the impact. We'll keep you updated.

We've ran all affected tests and there are ~23 places that would require a cleanup in Google's codebase. It looks feasible to prepare Google's codebase to this change, but we are trying to evaluate the impact on 3rd party libraries as well. We'll keep this thread updated.

dylhunn · 2022-03-26T01:06:21Z

Catching up on some triage -- it seems we could provide a schematic which adds a cast anywhere one of these methods is called, WDYT?

request('POTS') -> request('POTS' as AnyForTypedHttpMethod)

We'd probably want to land #44363 first, then the migration and eliminate the string types.

thePunderWoman · 2024-06-05T15:12:25Z

@cexbrayat We're going to close this and #44363 for now. We totally see the benefit of the type strictness, but it's just too challenging for us to prioritize landing. As usual, thanks for the effort!

angular-automatic-lock-bot · 2024-07-06T00:09:22Z

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.}

google-cla bot added the cla: yes label Dec 1, 2021

pullapprove bot requested review from jessicajaniuk and petebacondarwin December 1, 2021 14:57

angular-robot bot added the flag: breaking change label Dec 1, 2021

cexbrayat commented Dec 1, 2021

View reviewed changes

cexbrayat requested a review from AndrewKushnir December 1, 2021 14:59

petebacondarwin reviewed Dec 1, 2021

View reviewed changes

jessicajaniuk suggested changes Dec 1, 2021

View reviewed changes

packages/common/http/src/client.ts Outdated

Copy link

Contributor

jessicajaniuk Dec 1, 2021

Choose a reason for hiding this comment

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

I agree with @petebacondarwin. This would really simplify things.

dylhunn added area: common/http Issues related to HTTP and HTTP Client cross-cutting: types labels Dec 1, 2021

ngbot bot added this to the Backlog milestone Dec 1, 2021

cexbrayat force-pushed the fix/request-matcher-method branch from 79a0060 to 8fc399e Compare December 1, 2021 18:56

jessicajaniuk reviewed Dec 1, 2021

View reviewed changes

cexbrayat mentioned this pull request Dec 3, 2021

fix(common): stricter type for http methods #44363

Closed

14 tasks

dylhunn self-requested a review March 26, 2022 01:07

josephperrott added detected: breaking change PR contains a commit with a breaking change and removed flag: breaking change labels Nov 14, 2022

jessicajaniuk mentioned this pull request Jan 18, 2023

fix(http): Set Accept based on responseType #48529

Closed

14 tasks

thePunderWoman closed this Jun 5, 2024

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 6, 2024

fix(common): stricter type for http methods #44328

fix(common): stricter type for http methods #44328

Uh oh!

Conversation

cexbrayat commented Dec 1, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cexbrayat commented Dec 1, 2021

Uh oh!

jessicajaniuk commented Dec 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Dec 2, 2021

Uh oh!

cexbrayat commented Dec 3, 2021

Uh oh!

cexbrayat commented Dec 3, 2021

Uh oh!

AndrewKushnir commented Dec 7, 2021

Uh oh!

dylhunn commented Mar 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thePunderWoman commented Jun 5, 2024

Uh oh!

angular-automatic-lock-bot bot commented Jul 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dylhunn commented Mar 26, 2022 •

edited

Loading