-
Notifications
You must be signed in to change notification settings - Fork 20.6k
Ajax: Integrated native datatype, for XHR2 rich responseType support #1525
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
Conversation
…('arraybuffer', 'blob', 'document', 'json').
…tra identation for the long condition in line 98.
I like what I'm seeing here. Be careful about missing leading space at the beginning of some of your comments. I'll let others chime in. FYI, the forthcoming |
@jaubourg thank you for replying. I have added the leading spaces to the comments. Regarding the forthcoming $.xhr. Please, can you give me more information (A link for example) ? |
Here is the bug report: http://bugs.jquery.com/ticket/14509 It's early stage for now, I'm working on it and nothing's published yet. |
// Accessing binary-data responseText throws an exception | ||
// (#11426) | ||
if ( (!xhr.responseType || xhr.responseType === "text") && | ||
typeof xhr.responseText === "string" ) { |
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.
This line gets an extra level of indentation to differentiate from the body.
@scottgonzalez I have included your remarks. Please, take a look if everything is well. |
@jaubourg, understood. I asked because I had a little issue with the responseText attribute. It generates an InvalidStateError, if the responseType is not "text" (See : https://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-responsetext-attribute). So, in the code, I added a condition for that. Also, the support of the responseType is not complete in some browsers, partcularly the one I have used. The responseType functionned well with arraybuffer in my browser, but it didn't recognize the document, json or blob. It returned undefined for the response. So, if you choose responseText, you would generate an exception. |
@jaubourg, I should add for the ticket you provided. I am more for a solution, that would abstract the XHR and provides a rich means of communication (Through websockets for example). From what I have read on the ticket, there is a will to remove the abstraction, as it is making some harms to the developers, who cannot do everything they want, due to that abstractions (The case of this pull request). And that abstraction is not adding value, as we are dealing, with a single communication means, XHR. I have seen in the jquery ajax code, that there is a notion of transports, which has the role of the communication means abstraction. But it is only used for XHR. Do you think, that there is a way to go in that path (Abstracting the communication means) ? I have seen in libraries such as Ember-data, such notion of transportation mechanisms through the notion of adapters. |
There are actually two other transports in jQuery core. I know of 3 or 4 others out there. I'm moving forward the decision that was taken by the team to remove the abstraction because it was felt the features provided didn't justify the size. I don't agree with this assessment. I'm actually convinced a simplistic solution is doomed to slowly evolve into a new I'm not convinced current abstraction is good enough anyway. We've had some bad surprises with standards as of late (especially the behavior of script tag injection being changed) that contradicted some design decision we took and made an already messy situation with our ajax options even messier. Hope that makes things a little clearer. |
@jaubourg. Thanks, you have clarified a lot of things. Please, can you send me if it is possible a link to the jQuery transports, and the others ? Thanks a gain. And sorry again for my questions. |
Only one other in core: https://github.com/jquery/jquery/blob/master/src/ajax/script.js#L32 (forgot jsonp was just a prefilter). I personally coded:
There's also an iframe-based transport: https://github.com/cmlenz/jquery-iframe-transport/blob/master/jquery.iframe-transport.js#L92 and I can't find it right now but there was a transport-based mock ajax solution somewhere. There are several alternative xdr transports like https://github.com/MoonScript/jQuery-ajaxTransport-XDomainRequest/blob/master/jQuery.XDomainRequest.js#L1 and https://github.com/dkastner/jquery.iecors/blob/master/jquery.iecors.js#L1 |
Thanks, thanks a lot. It was just a rich discussion with you @jaubourg. I have learned a lot of things :). |
De rien, mon ami. Bon week-end ;) |
Merci, merci :). A vous aussi. |
strictEqual(data, "0123\n", "Check for response content"); | ||
} | ||
}]); | ||
|
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.
This block has missing spaces inside parens and brackets all over the place.
@scottgonzalez I have added the missing spaces. Is everything correct now ? |
deepEqual( [array[0], array[1], array[2], array[3]], [0, 1 ,2 , 3], "Check for response content" ); | ||
} | ||
}, { | ||
url: url("data/native.txt"), |
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.
missing spaces
@scottgonzalez please take a look at the last changes, and confirm. |
Looks good. |
Thanks @scottgonzalez :). @jaubourg @scottgonzalez I wanted to add a feature to this pull request, but I'd prefer to see it merged to add something else. It's about providing the native responseXML, through this native datatype. I'll discuss it in detail when the current pull request is merged. |
I like this too. Since it's a feature I think it's appropriate for 1.12/2.2. We'll also need some documentation for this before it lands, who's up for that? You can do a pull request over at https://github.com/jquery/api.jquery.com/ . To me the |
@dmethvin some carefull modifications are needed. First of all, there should be a description of the native datatype, and what kind of responses it provides. Secondly, the intelligent guess depends not only on the MIME type, but also on the XHR responseType field (The native response is guessed when the responseType field is defined and doesn't equal to "text"). Lastly, an example or two are necessary and would clarify the usage of the data type. I have cloned the jQuery API, and I will start writing and pull a request ASAP. |
@dmethvin @jaubourg @scottgonzalez, please take a look at jquery/api.jquery.com#463 for the documentation, that I have began writing. |
Any update on this guys? Why is this feature doomed to ignorance since 2012? I would really love to see responseType implemented in near future (not next two years!). |
Hello @ProgerXP, I am the author of this feature. It is planned to be integrated in jquery 1.12 (See milestone on the right), that for now haven't a release date, but it should be for the following months. I have worked a little on the doc as it is necessary, but for now I didn't had feedbacks regarding that (See jquery/api.jquery.com#463). I should resume working on the doc. Your feedbacks are wecome. |
@dmethvin, can you guide me on this ? I have started working on the doc, but didn't have any feedback. What should I do ? |
@tarikbenmerar We've been brainstorming in a gist here and a Google Doc here, please feel free to read and comment with your thoughts. It seems like this feature could be done pretty simply with an incoming option for |
Hello @dmethvin, sorry I didn't have time to focus on this topic. So, I am asking myself if jQuery ajax will exist in the next version with |
Yes, |
Hello @dmethvin. OK thanks, so should I work on the documentation of this feature ? I available ready to provide the remaining documentation for the feature to be ready for the next jquery version. I am asking myself when the next jquery version will be available, could you tell me please ? Also, I have left some feedbacks on your $.xhr GIST, perhaps you didn't receive a notification. Thanks in advance. |
@tarikbenmerar Unfortunately, Github doesn't seem to give notices on gists. Drives me crazy! We'd want to pick these changes into the 1.x-master branch as well. I see you've built in IE9 compat, any thoughts about how we'd treat this from a code/docs standpoint there? |
Offtopic comment: you might want to subscribe to the issue isaacs/github#21 where the (absent) gist notifications were discussed. |
Hello @dmethvin, I have analysed the differences in code between 1.x-master and master, it shouldn't be difficult to integrate this feature. So, I will create another pull request specifically for the 1.x-master. This should be the same for the documentation. For the gist, I have seen that for the third remark this has been taken by other contributors, and I am for those approaches. Please, consider the option of using lightweight thenables that can be coarsed into promises of any library. Tell me if I can provide an example. And for default settings, as you're dealing with xhr objects, you can easily create a factory, for generating xhrs with specific defaults. |
The jquery mainline is lagging on support for responseType: 'arraybuffer', an XHRHTTPRequest Level 2 feature (whatever that means). We use this responseType to transfer attachments. I rebased jquery/jquery#1525 on the 2.1.1 release and cut 2.1.1-ajax-nativetransport in my own fork.
I'm closing this for reasons similar to gh-1541 in that it adds complexity to |
@dmethvin OK Dave. I should just say that it would be impossible to integrate it into a third party plugin, because there needs change in the code (AJAX code currently interprets only textual data). If you want to provide the required feature in the plugin, you should manually copy paste from a string to an array buffer, a significant overhead (That's whay XHR2 is here) when you have 20 mega bytes, as in fact I do. I am still thinking that it is a needed feature (signalapp/Signal-Desktop@e156c2c). People are hardcoding AJAX requests to benefit from this basic feature. In any case, ..... I know I cannot influence this open source software. I'd wished I'd continue to integrate comfortably the merged feature in my PhD research code. Thanks, |
@tarikbenmerar My comment in gh-1541 is proposing a new API, not additional changes to |
@dmethvin sorry, I didn't know that such standard already exists (Fetch standard). I have taken a look at the W3C documentation, it has support for Array Buffer in a simple way. Now, if you mean simpler API, I guess it is $.xhr. Isn't it ? In this case, is there any additional thing I can contribute ? Just to know, if my responsibility ends here or if I can contribute something. |
We did some brainstorming but decided it was better to allow the solution to proceed as a plugin at its own pace, rather than being tied to our release schedule. There is some discussion in various places (here, here and here and here) but we haven't finalized the design and nobody is implementing it at the moment. |
@dmethvin OK, I will take a look at those links (I have seen some of them). If I am seeing it well, I should create a plugin for this solution, it will be another transport that manages native dataType, isn't it (with some additional converters and responseFields of course). ? |
I would think it starts as a standalone plugin based on the fetch polyfill, or perhaps on XHR2. There will be more bikeshedding on the API than actual code in the file I think. |
@dmethvin had some free time and worked on a plugin https://github.com/acigna/jquery-ajax-native. The source code is here : https://github.com/acigna/jquery-ajax-native/blob/master/src/jquery-ajax-native.js . I had to copy past a large part of the xhr ajaxTransport as there were no alternative. I hope there is a better way to add this support in the future. |
@ProgerXP here is for the plugin : https://github.com/acigna/jquery-ajax-native, if you want to use the feature now. |
XHR2 provides a response attribute, that contains a converted response to a native object, depending on the responseType ("arraybuffer", "blob", "document", "json"). See:
https://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-response-attribute
http://www.html5rocks.com/en/tutorials/file/xhr2/?redirect_from_locale=fr
I propose in this pull request, to create a native datatype and response, to manage this kind of response. Some of previous closed tickets proposed to include a binary dataType, with adhoc changes on the transport function :
http://bugs.jquery.com/ticket/11461
http://bugs.jquery.com/ticket/9992
http://bugs.jquery.com/ticket/14068
I think it is not appropriate as the response attribute deals with more than binary data, like arraybuffer, but extends to blob, document and json datatype. In my pull request, I create a native datatype to support all the four response types that are abstracted.
I think also that such improvements should be included as this is a new improvement in XHR, that is not already included in jQuery. Also, trying to use a converter in the case of ArrayBuffer for example, leads to a heavy loop for the conversion (See: http://updates.html5rocks.com/2012/06/How-to-convert-ArrayBuffer-to-and-from-String). Simple casting doesn't function.
In my case, I have used Brain dMRI NifTI data formats that are 20MB in size, and all of them need parsing after downloading, which is also a heavy computation (a hundred of milliseconds).
Also, treating a binary data as text, it is just not natural at all, as this is already the case.
In any way, I think we should find a solution, and this is my proposition. Please, take a look at the tests, for the usage. And let's start a discussion around this.