Extend text.get to be able to receieve request headers #38

Closed
DimitarChristoff opened this Issue Feb 1, 2013 · 3 comments

Comments

Projects
None yet
2 participants
@DimitarChristoff
Contributor

DimitarChristoff commented Feb 1, 2013

the text plugin is being used by the json plugin (correctly or not). unfortunately, some restful servers depend on the accept header to determine the content-type of the response and may default to say, XML.

The issue is that the text plugin exposes no way to set request headers at present.

I know you can hack via config: { text: { onXhr: function(xhr){ ... } } } but the json plugin has no direct access to that. It will be much more useful to change text.get to accept an optional 4th argument headers (obj) and set these on the xhr instance before the open.

A quick hack that worked:

        text.get = function (url, callback, errback, headers) {
            var xhr = text.createXhr(), header;
            xhr.open('GET', url, true);

            if (headers && headers === Object(headers)) {
                for (header in headers) {
                    if (headers.hasOwnProperty(header)) {
                        xhr.setRequestHeader(header.toLowerCase(), headers[header]);
                    }
                }
            }
   ...

and in json.js just pass as 4th argument.

{
    Accept: 'application/json'
}

I'd do a pull request but I cannot figure if this is the best API to do it or how to mimic the same for rhino file stream.

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke Feb 1, 2013

Member

This seems fine. I am open to a pull request, however, I would just use if (headers) { and skip the Object check. For the non-xhr paths, it is fine to leave them as they are, since it does not make sense in those cases.

Member

jrburke commented Feb 1, 2013

This seems fine. I am open to a pull request, however, I would just use if (headers) { and skip the Object check. For the non-xhr paths, it is fine to leave them as they are, since it does not make sense in those cases.

@DimitarChristoff

This comment has been minimized.

Show comment
Hide comment
@DimitarChristoff

DimitarChristoff Feb 2, 2013

Contributor

cool. what of fallback to config.text.requestHeaders if no arg? good idea?
can work with CORS etc

On Friday, February 1, 2013, James Burke wrote:

This seems fine. I am open to a pull request, however, I would just use if
(headers) { and skip the Object check. For the non-xhr paths, it is fine
to leave them as they are, since it does not make sense in those cases.


Reply to this email directly or view it on GitHubhttps://github.com/requirejs/text/issues/38#issuecomment-13007879.

Dimitar Christoff

"JavaScript is to JAVA what hamster is to ham"
@D_mitar - https://github.com/DimitarChristoff

Contributor

DimitarChristoff commented Feb 2, 2013

cool. what of fallback to config.text.requestHeaders if no arg? good idea?
can work with CORS etc

On Friday, February 1, 2013, James Burke wrote:

This seems fine. I am open to a pull request, however, I would just use if
(headers) { and skip the Object check. For the non-xhr paths, it is fine
to leave them as they are, since it does not make sense in those cases.


Reply to this email directly or view it on GitHubhttps://github.com/requirejs/text/issues/38#issuecomment-13007879.

Dimitar Christoff

"JavaScript is to JAVA what hamster is to ham"
@D_mitar - https://github.com/DimitarChristoff

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke Feb 2, 2013

Member

I would not do anything by default, just do what it does today. CORS works "automatically" if the user tells the text plugin to use an xhr via useXhr.

Member

jrburke commented Feb 2, 2013

I would not do anything by default, just do what it does today. CORS works "automatically" if the user tells the text plugin to use an xhr via useXhr.

@DimitarChristoff DimitarChristoff referenced this issue in millermedeiros/requirejs-plugins Feb 3, 2013

Closed

json plugin does not send the correct accepts content type #16

@jrburke jrburke closed this in 9c5d863 Feb 3, 2013

jrburke added a commit that referenced this issue Feb 3, 2013

Merge pull request #39 from DimitarChristoff/issue-38-headers
fixes #38 - gives access to xhr request headers via the .get api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment