Skip to content

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

Closed

Conversation

tarikbenmerar
Copy link

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.

@jaubourg
Copy link
Member

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 $.xhr uses xhr.response and only use xhr.reponseText if the former is not available.

@tarikbenmerar
Copy link
Author

@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) ?

@jaubourg
Copy link
Member

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" ) {
Copy link
Member

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.

@tarikbenmerar
Copy link
Author

@scottgonzalez I have included your remarks. Please, take a look if everything is well.

@tarikbenmerar
Copy link
Author

@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.

@tarikbenmerar
Copy link
Author

@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.

@jaubourg
Copy link
Member

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 $.ajax with the exact same drawbacks in the end (because $.ajax already was a simplistic, "un-designed", solution at the start) but I will respect the decision of the majority.

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.

@tarikbenmerar
Copy link
Author

@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.

@jaubourg
Copy link
Member

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

@tarikbenmerar
Copy link
Author

Thanks, thanks a lot. It was just a rich discussion with you @jaubourg. I have learned a lot of things :).

@jaubourg
Copy link
Member

De rien, mon ami. Bon week-end ;)

@tarikbenmerar
Copy link
Author

Merci, merci :). A vous aussi.

strictEqual(data, "0123\n", "Check for response content");
}
}]);

Copy link
Member

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.

@tarikbenmerar
Copy link
Author

@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"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing spaces

@tarikbenmerar
Copy link
Author

@scottgonzalez please take a look at the last changes, and confirm.

@scottgonzalez
Copy link
Member

Looks good.

@tarikbenmerar
Copy link
Author

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.

@dmethvin dmethvin added this to the 1.12/2.2 milestone Mar 1, 2014
@dmethvin
Copy link
Member

dmethvin commented Mar 1, 2014

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 $.xhr discussion is separate, I think we'll want to support XHR2 there obviously but it's a different API.

@tarikbenmerar
Copy link
Author

@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.

@tarikbenmerar
Copy link
Author

@dmethvin @jaubourg @scottgonzalez, please take a look at jquery/api.jquery.com#463 for the documentation, that I have began writing.

@ProgerXP
Copy link

ProgerXP commented Jul 9, 2014

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!).

@tarikbenmerar
Copy link
Author

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.

@tarikbenmerar
Copy link
Author

@dmethvin, can you guide me on this ? I have started working on the doc, but didn't have any feedback. What should I do ?

@dmethvin
Copy link
Member

dmethvin commented Jul 9, 2014

@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 responseType, assuming the design doesn't process the response by default (which it doesn't at the moment).

@tarikbenmerar
Copy link
Author

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 . x h r a s t h e u n d e r l y i n g t r a n s p o r t m e c h a n i s m o r n o t . I a m r e a l l y i n t e r r e s t e d t h a t t h i s f e a t u r e w i l l e x i s t . A t l e a s t , t o h a v e m y c o d e t o w o r k ( a n d n o t r e l y o n a c u s t o m c o d e I a m u s i n g ) . A n d I t h i n k , t h e r e w i l l b e p e o p l e i n t e r r e s t e d i n i t a s t h e y a r e c o m f o r t a b l e w i t h a j a x m e c h a n i s m f o r n o w , u n t i l l t h e r e i s a t r a n s i t i o n .xhr. Please, should I invest on the documentation ?

@dmethvin
Copy link
Member

Yes, $.ajax() will be there and won't go away for years, if ever. We're just looking for a simpler interface that will support more advanced features, and that is $.xhr().

@tarikbenmerar
Copy link
Author

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.

@dmethvin
Copy link
Member

@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?

@Mithgol
Copy link

Mithgol commented Oct 21, 2014

Offtopic comment: you might want to subscribe to the issue isaacs/github#21 where the (absent) gist notifications were discussed.

@tarikbenmerar
Copy link
Author

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.

liliakai added a commit to signalapp/Signal-Desktop that referenced this pull request Oct 30, 2014
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.
@dmethvin
Copy link
Member

dmethvin commented Dec 4, 2014

I'm closing this for reasons similar to gh-1541 in that it adds complexity to $.ajax for one transport and requires additional documentation to an API call that is already the largest surface area. It would be great to see a fetch-based plugin that we could use as a simple replacement for these applications and eventually integrate into jQuery itself.

@dmethvin dmethvin closed this Dec 4, 2014
@tarikbenmerar
Copy link
Author

@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,

@dmethvin
Copy link
Member

dmethvin commented Dec 4, 2014

@tarikbenmerar My comment in gh-1541 is proposing a new API, not additional changes to $.ajax. Documenting the additions is at least as difficult as the code, and that work has yet to be done. A simpler XHR-only (or fetch-only) API would be much easier to document.

@tarikbenmerar
Copy link
Author

@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.

@dmethvin
Copy link
Member

dmethvin commented Dec 4, 2014

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.

@tarikbenmerar
Copy link
Author

@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). ?

@dmethvin
Copy link
Member

dmethvin commented Dec 5, 2014

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.

@tarikbenmerar
Copy link
Author

@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.

@tarikbenmerar
Copy link
Author

@ProgerXP here is for the plugin : https://github.com/acigna/jquery-ajax-native, if you want to use the feature now.

@mgol mgol removed this from the 3.0.0 milestone Oct 17, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants