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

Improved ajax #171

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
@pangratz
Member

pangratz commented Oct 11, 2016

Rendered

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Oct 11, 2016

If we're going through this effort is it worth trying to build something that decouples us from the underlying jQuery implementation? There is hopefully a future in which we don't ship that by default and it would be sad to require jQuery just for $.ajax.

See: https://github.com/ember-cli/ember-ajax

nathanhammond commented Oct 11, 2016

If we're going through this effort is it worth trying to build something that decouples us from the underlying jQuery implementation? There is hopefully a future in which we don't ship that by default and it would be sad to require jQuery just for $.ajax.

See: https://github.com/ember-cli/ember-ajax

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Oct 11, 2016

Member

@nathanhammond yep, I definitely agree. The hooks should be designed to be agnostic of the specific library/implementation which does the actual request. I forgot to mention that explicitly in the RFC...

Member

pangratz commented Oct 11, 2016

@nathanhammond yep, I definitely agree. The hooks should be designed to be agnostic of the specific library/implementation which does the actual request. I forgot to mention that explicitly in the RFC...

@mkay581

This comment has been minimized.

Show comment
Hide comment
@mkay581

mkay581 commented Oct 11, 2016

pangratz added some commits Oct 18, 2016

Minor improvements
- `makeRequest` is public
- smaller adjustments
@tylerturdenpants

This comment has been minimized.

Show comment
Hide comment
@tylerturdenpants

tylerturdenpants Oct 20, 2016

Member

Grammar error. "a" instead of "an"

Member

tylerturdenpants commented on text/0000-ember-data-improved-ajax.md in df4e1f1 Oct 20, 2016

Grammar error. "a" instead of "an"

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Oct 20, 2016

Member

Thanks!

Member

pangratz replied Oct 20, 2016

Thanks!

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Oct 25, 2016

Member

I think the hooks as described in this RFC have no dependency on $.ajax. They are some examples based on $.ajax usage, men we could remove them, or add more examples with fetch for example.

One suggestion I have is that we are passing to makeRequest() a hash of options. The hash looks like this:

{
  url: 'https://api.com/posts',
  method: 'POST',
  headers: {},
  body: '{ post: {} }'
}

There is a spec for a Request object (as part of fetch). I am not saying we should use it directly, but, I think we should model a Request object in ember-data after it. DS.Request? I would also expect both be compatible and so once we support fetch, it would be possible to pass in native Request object.

Member

tchak commented Oct 25, 2016

I think the hooks as described in this RFC have no dependency on $.ajax. They are some examples based on $.ajax usage, men we could remove them, or add more examples with fetch for example.

One suggestion I have is that we are passing to makeRequest() a hash of options. The hash looks like this:

{
  url: 'https://api.com/posts',
  method: 'POST',
  headers: {},
  body: '{ post: {} }'
}

There is a spec for a Request object (as part of fetch). I am not saying we should use it directly, but, I think we should model a Request object in ember-data after it. DS.Request? I would also expect both be compatible and so once we support fetch, it would be possible to pass in native Request object.

let headers = this.headersForRequest(params);
let body = this.bodyForRequest(params);
return RSVP.hash({ method, url, headers, body });

This comment has been minimized.

@bmac

bmac Oct 25, 2016

Member

Should _requestFor be made a public method in case the user wants to add some other config property?

@bmac

bmac Oct 25, 2016

Member

Should _requestFor be made a public method in case the user wants to add some other config property?

This comment has been minimized.

@tchak

tchak Oct 25, 2016

Member

and it could return a "standard" Request object :)

@tchak

tchak Oct 25, 2016

Member

and it could return a "standard" Request object :)

@param request Object containing all properties for the request which should be made
@return Promise
*/
makeRequest(request) {

This comment has been minimized.

@bmac

bmac Oct 25, 2016

Member

I think we should enumerate all of the properties Ember Data expects/supports on the request object.

@bmac

bmac Oct 25, 2016

Member

I think we should enumerate all of the properties Ember Data expects/supports on the request object.

This comment has been minimized.

@pangratz

pangratz Oct 25, 2016

Member

Good point!

@pangratz

pangratz Oct 25, 2016

Member

Good point!

This comment has been minimized.

@bmac

bmac Oct 25, 2016

Member

Based on my reading of this rfc it looks like it supports url, method, headers and body. With the exception of url all of these properties are shared by the 2nd argument to fetch (See: https://fetch.spec.whatwg.org/#requestinit). Which may lead a user to believe it supports all the properties that fetch supports.

This leads me to ask 2 questions.

  1. How much of the RequestInit api should Ember Data implement?
  2. Should the url property be pulled out of the request object and makeRequest API be change to align more closely with the fetch api? makeRequest(url, requestInit)
@bmac

bmac Oct 25, 2016

Member

Based on my reading of this rfc it looks like it supports url, method, headers and body. With the exception of url all of these properties are shared by the 2nd argument to fetch (See: https://fetch.spec.whatwg.org/#requestinit). Which may lead a user to believe it supports all the properties that fetch supports.

This leads me to ask 2 questions.

  1. How much of the RequestInit api should Ember Data implement?
  2. Should the url property be pulled out of the request object and makeRequest API be change to align more closely with the fetch api? makeRequest(url, requestInit)

This comment has been minimized.

@tchak

tchak Oct 25, 2016

Member

fetch can also take a single argument with Request instance. This is kinda what I was proposing in my other comments. But align makeRequest with fetch options is 👍

@tchak

tchak Oct 25, 2016

Member

fetch can also take a single argument with Request instance. This is kinda what I was proposing in my other comments. But align makeRequest with fetch options is 👍

@bmac

This comment has been minimized.

Show comment
Hide comment
@bmac

bmac Oct 26, 2016

Member

@tchak I'm not sold on a DS.Request class. What is the advantage of that wrapping the request state in a class instance vs just having users return a plain object with the same properties?

Member

bmac commented Oct 26, 2016

@tchak I'm not sold on a DS.Request class. What is the advantage of that wrapping the request state in a class instance vs just having users return a plain object with the same properties?

@bmac

This comment has been minimized.

Show comment
Hide comment
@bmac

bmac Oct 26, 2016

Member

@pangratz in response to you unresolved questions section at the end of the rfc.

currently requestType is used to indicate the specific adapter operation, but that name might not be to expressive. Should this be renamed to the more verbose, like adapterOperation?

I'm in favor of renaming requestType to adapterOperation.

should queryForRequest be named queryParamsForRequest?

Again I like this change but I don't feel very strongly about it.

should there be more hooks within urlForRequest, as outlined in the Syntax section on the URLs wiki page?

I do not think we need any more hooks inside urlForRequest other then what is described in this rfc. I believe the current hooks satisfy most of not all of the requests for customization points in the url that we've seen in the Ember Data repo over the years. I suspect if we added more hooks they would be unused in 99% of cases and the remaining cases users can still override urlForRequest to generate the url they need.

  • the currently proposed API doesn't allow to only use public API to customize the json-api-adapter, though the current implementation could be changed to set the Content-Type header instead (which would be more agnostic anyway). This might be an indicator that the proposed API still needs some adjustments so json-api adapter uses only public API.
  • what hooks are missing?
    • setting the dataType #4357?

I was originally thinking _requestFor should be made public so users can return other config options that the fetch api supports besides headers, method and body. But that doesn't solve the case of special jquery specific configuration.

2 possible approches:

  1. Limit the ajax adapter to only allow configuring of jQuery ajax features that can be described using a fetch api. (For example contentType would use the Content-Type header but configuring dataType would be unsupported.) The rational being this would make it easier to move the rest adapter to a service worker some day since xhr isn't supported inside a service worker but fetch is.
  2. Add a jqueryAjaxOptions() method and document that it will only be called when jQuery is being used for makeRequest() (Possibly difficult to detect).
Member

bmac commented Oct 26, 2016

@pangratz in response to you unresolved questions section at the end of the rfc.

currently requestType is used to indicate the specific adapter operation, but that name might not be to expressive. Should this be renamed to the more verbose, like adapterOperation?

I'm in favor of renaming requestType to adapterOperation.

should queryForRequest be named queryParamsForRequest?

Again I like this change but I don't feel very strongly about it.

should there be more hooks within urlForRequest, as outlined in the Syntax section on the URLs wiki page?

I do not think we need any more hooks inside urlForRequest other then what is described in this rfc. I believe the current hooks satisfy most of not all of the requests for customization points in the url that we've seen in the Ember Data repo over the years. I suspect if we added more hooks they would be unused in 99% of cases and the remaining cases users can still override urlForRequest to generate the url they need.

  • the currently proposed API doesn't allow to only use public API to customize the json-api-adapter, though the current implementation could be changed to set the Content-Type header instead (which would be more agnostic anyway). This might be an indicator that the proposed API still needs some adjustments so json-api adapter uses only public API.
  • what hooks are missing?
    • setting the dataType #4357?

I was originally thinking _requestFor should be made public so users can return other config options that the fetch api supports besides headers, method and body. But that doesn't solve the case of special jquery specific configuration.

2 possible approches:

  1. Limit the ajax adapter to only allow configuring of jQuery ajax features that can be described using a fetch api. (For example contentType would use the Content-Type header but configuring dataType would be unsupported.) The rational being this would make it easier to move the rest adapter to a service worker some day since xhr isn't supported inside a service worker but fetch is.
  2. Add a jqueryAjaxOptions() method and document that it will only be called when jQuery is being used for makeRequest() (Possibly difficult to detect).
warning("use queryForRequest to specify query params");
} else if (query) {
let sortedQueryParams = this.sortQueryParams(query);
let queryParams = Ember.$.param(sortedQueryParams);

This comment has been minimized.

@bmac

bmac Dec 13, 2016

Member

I think we may want to expose a public method for serializing query params that users can override if needed.

@bmac

bmac Dec 13, 2016

Member

I think we may want to expose a public method for serializing query params that users can override if needed.

This comment has been minimized.

@pangratz

pangratz Dec 14, 2016

Member

You mean additionally to queryForRequest?

@pangratz

pangratz Dec 14, 2016

Member

You mean additionally to queryForRequest?

This comment has been minimized.

@bmac

bmac Dec 14, 2016

Member

Yes. Something that transforms the object returned from queryForRequest() into a string.

@bmac

bmac Dec 14, 2016

Member

Yes. Something that transforms the object returned from queryForRequest() into a string.

This comment has been minimized.

@pangratz

pangratz Dec 14, 2016

Member

Hmm, I see. Maybe queryStringForRequest()?

But then we would have 5 hooks which are involved getting the URL for a request:

  • urlForRequest
    • pathForRequest
    • queryForRequest
    • sortQueryParams
    • queryStringForRequest

For now I am not convinced that an additional hook is necessary. Can you think of an use case where such a hook would come in handy?

@pangratz

pangratz Dec 14, 2016

Member

Hmm, I see. Maybe queryStringForRequest()?

But then we would have 5 hooks which are involved getting the URL for a request:

  • urlForRequest
    • pathForRequest
    • queryForRequest
    • sortQueryParams
    • queryStringForRequest

For now I am not convinced that an additional hook is necessary. Can you think of an use case where such a hook would come in handy?

This comment has been minimized.

@bmac

bmac Dec 14, 2016

Member

I've seen a few comments on slack recently were people want arrays or nested query params to be serialized differently from how $.param does it.

I wonder how widely used sortQueryParams is and I wonder if we could deprecate that and let users do that work in queryForRequest or queryStringForRequest if they need it.

@bmac

bmac Dec 14, 2016

Member

I've seen a few comments on slack recently were people want arrays or nested query params to be serialized differently from how $.param does it.

I wonder how widely used sortQueryParams is and I wonder if we could deprecate that and let users do that work in queryForRequest or queryStringForRequest if they need it.

This comment has been minimized.

@pangratz

pangratz Dec 14, 2016

Member

Ah, arrays and nested queries are definitely a valid use case 👍

Deprecating sortQueryParams in favor of queryStringForRequest sounds reasonable to me!

@pangratz

pangratz Dec 14, 2016

Member

Ah, arrays and nested queries are definitely a valid use case 👍

Deprecating sortQueryParams in favor of queryStringForRequest sounds reasonable to me!

##### `methodForRequest`
Invoked to get the HTTP method for the request. Should return one of `"GET"`,
`"PUT"`, `"POST"` or `"DELETE"`.

This comment has been minimized.

@woprandi

woprandi Mar 9, 2017

Why not "PATCH" ?

@woprandi

woprandi Mar 9, 2017

Why not "PATCH" ?

@amiel

This comment has been minimized.

Show comment
Hide comment
@amiel

amiel Mar 14, 2017

This will help resolve some issues we've had with ember-data-url-templates :)

amiel commented Mar 14, 2017

This will help resolve some issues we've had with ember-data-url-templates :)

bartocc pushed a commit to bartocc/ember-simple-auth that referenced this pull request Apr 25, 2017

Julien Palmas
Update data-adapter-mixin.js
Fix incorrect docs about headersForRequest being used for ember-data versions >= 2.7.
This is on hold until emberjs/rfcs#171 gets resolved

marcoow added a commit to simplabs/ember-simple-auth that referenced this pull request Jun 7, 2017

Update data-adapter-mixin.js
Fix incorrect docs about headersForRequest being used for ember-data versions >= 2.7.
This is on hold until emberjs/rfcs#171 gets resolved

marcoow added a commit to simplabs/ember-simple-auth that referenced this pull request Jun 7, 2017

Update data-adapter-mixin.js
Fix incorrect docs about headersForRequest being used for ember-data versions >= 2.7.
This is on hold until emberjs/rfcs#171 gets resolved

marcoow added a commit to simplabs/ember-simple-auth that referenced this pull request Jun 7, 2017

Update DataAdapterMixin docs (#1366)
* Update data-adapter-mixin.js

Fix incorrect docs about headersForRequest being used for ember-data versions >= 2.7.
This is on hold until emberjs/rfcs#171 gets resolved

* Update data-adapter-mixin.js
@cibernox

This comment has been minimized.

Show comment
Hide comment
@cibernox

cibernox Jun 7, 2017

Contributor

This seems to have fallen idle. This would be much needed in order the build a reasonable compatibility layer to be able to swap jQuery by just raw fetch.
At the moment adapters setup headers using the beforeSend function on ajax, which has no fetch counterpart and it is hard, if not plain impossible, to introspect.

Is there any open question or significant pushback from anyone that prevents this from advancing?

Contributor

cibernox commented Jun 7, 2017

This seems to have fallen idle. This would be much needed in order the build a reasonable compatibility layer to be able to swap jQuery by just raw fetch.
At the moment adapters setup headers using the beforeSend function on ajax, which has no fetch counterpart and it is hard, if not plain impossible, to introspect.

Is there any open question or significant pushback from anyone that prevents this from advancing?

pichfl added a commit to pichfl/ember-simple-auth that referenced this pull request Jun 8, 2017

Update DataAdapterMixin docs (#1366)
* Update data-adapter-mixin.js

Fix incorrect docs about headersForRequest being used for ember-data versions >= 2.7.
This is on hold until emberjs/rfcs#171 gets resolved

* Update data-adapter-mixin.js
@denzo

This comment has been minimized.

Show comment
Hide comment
@denzo

denzo Oct 11, 2017

Same as above comment. It has been a while, it would be great to know where it's at or where it is on the timeline.

Will be happy to contribute to speed it up, if anything at all is needed.

The fact that https://emberjs.com/api/ember-data/2.16/classes/DS.RESTAdapter/methods/headersForRequest?anchor=headersForRequest is still in the docs but can not be used isn't ideal.

@pangratz @bmac @tchak

denzo commented Oct 11, 2017

Same as above comment. It has been a while, it would be great to know where it's at or where it is on the timeline.

Will be happy to contribute to speed it up, if anything at all is needed.

The fact that https://emberjs.com/api/ember-data/2.16/classes/DS.RESTAdapter/methods/headersForRequest?anchor=headersForRequest is still in the docs but can not be used isn't ideal.

@pangratz @bmac @tchak

@sandstrom sandstrom referenced this pull request Jan 18, 2018

Open

[QUEST] Making jQuery Optional #5320

2 of 6 tasks complete
@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Jan 21, 2018

Member

Her is the list of remarks and concerns so far on this RFC:

  • make requestFor public
  • the RFC dose not provide a way to customise json-api adapter with public methods just set Content-Type header (?)
  • queryForRequest -> queryParamsForRequest
  • deprecating sortQueryParams in favour of queryStringForRequest

"make things more standard":

  • return Request object from requestFor (?)
  • makeRequest to take Request (?)
  • return a URLSearchParams from queryParamsForRequest (?)
Member

tchak commented Jan 21, 2018

Her is the list of remarks and concerns so far on this RFC:

  • make requestFor public
  • the RFC dose not provide a way to customise json-api adapter with public methods just set Content-Type header (?)
  • queryForRequest -> queryParamsForRequest
  • deprecating sortQueryParams in favour of queryStringForRequest

"make things more standard":

  • return Request object from requestFor (?)
  • makeRequest to take Request (?)
  • return a URLSearchParams from queryParamsForRequest (?)
@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Jan 22, 2018

Member

@emberjs/ember-data-contributors @igorT what do we need to do to make progress on these questions?

Member

wycats commented Jan 22, 2018

@emberjs/ember-data-contributors @igorT what do we need to do to make progress on these questions?

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom Feb 2, 2018

Regarding headersForRequest, I think it would be good if it returned a promise that resolved to a hash of headers (i.e. any consumer must expect either an object or a promise).

It's common to append authentication/session tokens in headers, and they may be stored asynchronously (for example in IndexedDB).

sandstrom commented Feb 2, 2018

Regarding headersForRequest, I think it would be good if it returned a promise that resolved to a hash of headers (i.e. any consumer must expect either an object or a promise).

It's common to append authentication/session tokens in headers, and they may be stored asynchronously (for example in IndexedDB).

@fivetanley

This comment has been minimized.

Show comment
Hide comment
@fivetanley

fivetanley Feb 16, 2018

Member

return a URLSearchParams from queryParamsForRequest

@tchak is this polyfillable? This isn't supported by Internet Explorer natively.

Member

fivetanley commented Feb 16, 2018

return a URLSearchParams from queryParamsForRequest

@tchak is this polyfillable? This isn't supported by Internet Explorer natively.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jun 15, 2018

Member

Where are we on this? Is this still a direction we would like to go?

Member

rwjblue commented Jun 15, 2018

Where are we on this? Is this still a direction we would like to go?

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Jun 15, 2018

Member

@rwjblue I think we want to finish migration to fetch in ember-data before adding anything else to the network layer. For what it worth, I implemented this RFC (and other things) in an addon https://github.com/tchak/ember-fetch-adapter

Member

tchak commented Jun 15, 2018

@rwjblue I think we want to finish migration to fetch in ember-data before adding anything else to the network layer. For what it worth, I implemented this RFC (and other things) in an addon https://github.com/tchak/ember-fetch-adapter

@runspired

This comment has been minimized.

Show comment
Hide comment
@runspired

runspired Jun 27, 2018

Contributor

Closing as we are definitely moving in a very different direction. Keep your eyes open for a new RFC :)

Contributor

runspired commented Jun 27, 2018

Closing as we are definitely moving in a very different direction. Keep your eyes open for a new RFC :)

@runspired runspired closed this Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment