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

refactor(ajax): Move to simple Observable implementation. Fix errors. #5661

Merged
merged 8 commits into from
Sep 3, 2020

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Aug 20, 2020

  • Refactors ajax to use a simple Observable implementation.
  • Moves code to a new ajax-specific folder.
  • Adds a lot more documentation to classes and support classes.
  • Differentiates between the configuration passed to ajax (AjaxConfig) and the request values used to make the HTTP request (AjaxRequest), as the latter has more required values.
  • Ensures that no configuration values are mutated while making the request. (resolves Observable Ajax POST with wrong Content-Type after PUT #2801)
  • Ensures there is at least one valid test for ajax.patch. The old one was sketchy.
  • Adds better comments throughout the code.
  • Adds better typing to ajax functions.
  • Also ensures x-requested-with header is set properly
  • Adds comments
  • Code clean up
  • Normalize headers to be lowercase, since they are case insensitive.
  • Updates tests
  • Remove superfluous checks in unsubscribe
  • Ajax will now allow default behavior as defined in the specs:
  • if the body is set to: Blob, ArrayBuffer, any array buffer view (like a byte sequence, e.g. Uint8Array, etc), FormData, URLSearchParams, string, or ReadableStream, default handling is use. If the body is otherwise typeof "object", then it will be converted to JSON via JSON.stringify, and the Content-Type header will be set to application/json;charset=utf-8. All other types will emit an error. (resolves AJAX Content-Type Handling. #2837)
  • Puts the behavior in-line with Axios, et al.

BREAKING CHANGE: For TypeScript users, AjaxRequest is no longer the type that should be explicitly used to create an ajax. It is now AjaxConfig, although the two types are compatible, only AjaxConfig has progressSubscriber and createXHR.

BREAKING CHANGE: ajax body serialization will now use default XHR behavior in all cases. If the body is a Blob, ArrayBuffer, any array buffer view (like a byte sequence, e.g. Uint8Array, etc), FormData, URLSearchParams, string, or ReadableStream, default handling is use. If the body is otherwise typeof "object", then it will be converted to JSON via JSON.stringify, and the Content-Type header will be set to application/json;charset=utf-8. All other types will emit an error.

BREAKING CHANGE: The Content-Type header passed to ajax configuration no longer has any effect on the serialization behavior of the AJAX request.

@benlesh benlesh requested a review from cartant August 20, 2020 16:44
src/internal/observable/dom/AjaxObservable.ts Outdated Show resolved Hide resolved
src/internal/observable/dom/AjaxObservable.ts Outdated Show resolved Hide resolved
src/internal/observable/dom/AjaxObservable.ts Outdated Show resolved Hide resolved
@benlesh benlesh changed the title fix(ajax): Do not mutate headers passed as arguments refactor(ajax): Move to simple Observable implementation. Fix errors. Aug 21, 2020
@benlesh benlesh requested a review from cartant August 22, 2020 17:22
src/internal/ajax/ajax.ts Outdated Show resolved Hide resolved
@benlesh
Copy link
Member Author

benlesh commented Sep 2, 2020

As I'm going through the backlog of issues, I keep finding stuff related to ajax, so I've added a couple more commits to fix another (nasty) issue.

@cartant
Copy link
Collaborator

cartant commented Sep 3, 2020

This also seems to need an API guardian update.

@benlesh
Copy link
Member Author

benlesh commented Sep 3, 2020

The API guardian is killing me. It seems like there's an issue where you have to rebase, THEN update api_guardian before it actually works. This is the third PR where I've had issues. The thing it's complaining about is unrelated to the PR.

- Also ensures x-requested-with header is set properly
- Adds comments
- Code clean up
- Normalize headers to be lowercase, since they are case insensitive.
- Updates tests
- Remove superfluous checks in unsubscribe

resolves ReactiveX#2801
- Refactors `ajax` to use a simple `Observable` implementation.
- Moves code to a new ajax-specific folder.
- Adds a lot more documentation to classes and support classes.
- Differentiates between the configuration passed to `ajax` (`AjaxConfig`) and the request values used to make the HTTP request (`AjaxRequest`), as the latter has more required values.
- Ensures that no configuration values are mutated while making the request.
- Ensures there is at least one valid test for `ajax.patch`. The old one was sketchy.
- Adds better comments throughout the code.
- Adds better typing to `ajax` functions.

BREAKING CHANGE: For TypeScript users, `AjaxRequest` is no longer the type that should be explicitly used to create an `ajax`. It is now `AjaxConfig`, although the two types are compatible, only `AjaxConfig` has `progressSubscriber` and `createXHR`.
…pe where possible

- Ajax will now allow default behavior as defined in the specs:
  - https://xhr.spec.whatwg.org/#the-send()-method
  - https://fetch.spec.whatwg.org/#concept-bodyinit-extract
- if the `body` is set to: `Blob`, `ArrayBuffer`, any array buffer view (like a byte sequence, e.g. `Uint8Array`, etc), `FormData`, `URLSearchParams`, `string`, or `ReadableStream`, default handling is use. If the `body` is otherwise `typeof` `"object"`, then it will be converted to JSON via `JSON.stringify`, and the `Content-Type` header will be set to `application/json;charset=utf-8`. All other types will emit an error.
- Updates tests.
- Puts the behavior in-line with Axios, et al.

resolves ReactiveX#2837

BREAKING CHANGE: `ajax` body serialization will now use default XHR behavior in all cases. If the body is a `Blob`, `ArrayBuffer`, any array buffer view (like a byte sequence, e.g. `Uint8Array`, etc), `FormData`, `URLSearchParams`, `string`, or `ReadableStream`, default handling is use. If the `body` is otherwise `typeof` `"object"`, then it will be converted to JSON via `JSON.stringify`, and the `Content-Type` header will be set to `application/json;charset=utf-8`. All other types will emit an error.

BREAKING CHANGE: The `Content-Type` header passed to `ajax` configuration no longer has any effect on the serialization behavior of the AJAX request.
@benlesh benlesh force-pushed the fix/2801-ajax-dont-mutate-headers branch from 7db988d to 68aa003 Compare September 3, 2020 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AJAX Content-Type Handling. Observable Ajax POST with wrong Content-Type after PUT
4 participants