-
Notifications
You must be signed in to change notification settings - Fork 405
Support for configuring the browser fetch credentials mode in BrowserClient.
#1937
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
15c7647
cf337e4
020f587
df5be03
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 |
|---|---|---|
| @@ -1,3 +1,7 @@ | ||
| ## 1.6.2-wip | ||
|
|
||
| * Add `BrowserCredentialsMode` and `BrowserClient.credentialsMode` to support the `omit` browser fetch credentials mode. Deprecate `withCredentials`. | ||
|
Collaborator
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. 80 columns please |
||
|
|
||
| ## 1.6.1-wip | ||
|
|
||
| * Clarified the behavior of response headers in API documentation comments. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,9 +35,39 @@ BaseClient createClient() { | |
|
|
||
| @JS('fetch') | ||
| external JSPromise<Response> _fetch( | ||
| RequestInfo input, [ | ||
| RequestInit init, | ||
| ]); | ||
| RequestInfo input, [ | ||
| RequestInit init, | ||
| ]); | ||
|
|
||
| /// The browser `fetch` credentials mode used by [BrowserClient]. | ||
| /// | ||
| /// Controls whether the browser sends credentials such as cookies, TLS client | ||
| /// certificates, or authorization headers with a request. | ||
| /// | ||
| /// See also: | ||
| /// - https://fetch.spec.whatwg.org/#requestcredentials | ||
| enum BrowserCredentialsMode { | ||
|
Collaborator
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. Do we want to call this RequestCredentials to match the spec? |
||
| /// Never send credentials with the request and never include credentials in | ||
| /// the response. | ||
| /// | ||
| /// This corresponds to the browser `fetch` credentials mode `omit`. | ||
| omit('omit'), | ||
|
|
||
| /// Send credentials for same-origin requests only. | ||
|
Collaborator
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. and only include credentials in same-origin replies. |
||
| /// | ||
| /// This corresponds to the browser `fetch` credentials mode `same-origin`. | ||
| sameOrigin('same-origin'), | ||
|
|
||
| /// Always send credentials, even for cross-origin requests. | ||
|
Collaborator
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. ...and include them in all responses. |
||
| /// | ||
| /// This corresponds to the browser `fetch` credentials mode `include`. | ||
| include('include'); | ||
|
|
||
| const BrowserCredentialsMode(this._value); | ||
|
|
||
| /// The value passed to the browser `fetch` `RequestInit.credentials` field. | ||
| final String _value; | ||
| } | ||
|
|
||
| /// A `package:web`-based HTTP client that runs in the browser and is backed by | ||
| /// [`window.fetch`](https://fetch.spec.whatwg.org/). | ||
|
|
@@ -52,11 +82,49 @@ external JSPromise<Response> _fetch( | |
| /// Responses are streamed but requests are not. A request will only be sent | ||
| /// once all the data is available. | ||
| class BrowserClient extends BaseClient { | ||
| /// Create a [BrowserClient]. | ||
| /// | ||
| /// By default, credentials are sent for same-origin requests only, which | ||
|
Collaborator
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 would remove. Most people will not be aware of the past behavior.
Author
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. Done, removed the reference to the past behavior to keep the documentation focused and clean. |
||
| /// matches the previous default behavior when [withCredentials] was `false`. | ||
| BrowserClient({ | ||
| this.credentialsMode = BrowserCredentialsMode.sameOrigin, | ||
| }); | ||
|
|
||
| /// The browser `fetch` credentials mode used for requests. | ||
| /// | ||
| /// Defaults to [BrowserCredentialsMode.sameOrigin], which matches the | ||
| /// previous behavior when [withCredentials] was `false`. | ||
| BrowserCredentialsMode credentialsMode; | ||
|
Collaborator
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. Can we make this private? I think that mutating it after construction is error-prone in any case.
Author
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 agree that mutating this after construction is error-prone. However, withCredentials was a mutable public field in the original code. Making requestCredentials final and removing or disabling the withCredentials setter would break backwards compatibility for users who configure the client after instantiation.
Collaborator
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. How about we make
Author
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. Sounds perfect! That's exactly how I've structured it in the latest push. The internal field is now private (_requestCredentials) and mutable via the deprecated withCredentials setter to preserve backwards compatibility. I've also exposed a public read-only getter (requestCredentials) so new users can read the mode without being able to mutate it after construction. When withCredentials is eventually removed, we can easily make the underlying field final. I think keeping the old setter mutable for now is a great call, as some users might rely on HTTP client generators (like OpenAPI Generator) that instantiate the client first and configure properties like credentials dynamically right after. This way, we don't break their existing workflows. |
||
|
|
||
| /// Whether to send credentials such as cookies or authorization headers for | ||
| /// cross-site requests. | ||
| /// | ||
| /// Defaults to `false`. | ||
| bool withCredentials = false; | ||
| /// | ||
| /// This property is deprecated because it can only represent two of the three | ||
| /// browser `fetch` credentials modes (`same-origin` and `include`). Use | ||
| /// [credentialsMode] instead to also support [BrowserCredentialsMode.omit]. | ||
| /// | ||
| /// Reading this property returns `true` only when [credentialsMode] is | ||
| /// [BrowserCredentialsMode.include]. | ||
| @Deprecated('Use credentialsMode instead.') | ||
| bool get withCredentials => | ||
| credentialsMode == BrowserCredentialsMode.include; | ||
|
|
||
| /// Whether to send credentials such as cookies or authorization headers for | ||
| /// cross-site requests. | ||
| /// | ||
| /// Setting this to `true` sets [credentialsMode] to | ||
| /// [BrowserCredentialsMode.include]. | ||
| /// | ||
| /// Setting this to `false` sets [credentialsMode] to | ||
| /// [BrowserCredentialsMode.sameOrigin]. | ||
| @Deprecated('Use credentialsMode instead.') | ||
| set withCredentials(bool value) { | ||
| credentialsMode = value | ||
| ? BrowserCredentialsMode.include | ||
| : BrowserCredentialsMode.sameOrigin; | ||
| } | ||
|
|
||
| bool _isClosed = false; | ||
| final _openRequestAbortControllers = <AbortController>[]; | ||
|
|
@@ -85,7 +153,7 @@ class BrowserClient extends BaseClient { | |
| RequestInit( | ||
| method: request.method, | ||
| body: bodyBytes.isNotEmpty ? bodyBytes.toJS : null, | ||
| credentials: withCredentials ? 'include' : 'same-origin', | ||
| credentials: credentialsMode._value, | ||
| headers: { | ||
| if (request.contentLength case final contentLength?) | ||
| 'content-length': contentLength, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| name: http | ||
| version: 1.6.1-wip | ||
| version: 1.6.2-wip | ||
|
Collaborator
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. You can revert this.
Author
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. ok |
||
| description: A composable, multi-platform, Future-based API for HTTP requests. | ||
| repository: https://github.com/dart-lang/http/tree/master/pkgs/http | ||
|
|
||
|
|
||
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.
1.6.1-wip (the
-wipmeans Work-In-Progress) has not yet been released so you don't need to add a new version - just add your note to the 1.6.1-wip section.