Rewrite transport #25

jharding · 2013-02-22T03:08:54Z

Looking for some feedback. This change would allow you to configure the ajax request so you could set headers, parse the response, etc. It should resolve #8, #13, #20.

My only concern is that this exposes too much. Rather than making every ajax option configurable, we could just whitelist the ones we think would be useful. Maybe exposing everything is the way to go though, after all who are we to deem which options are useful 😃.

vskarich · 2013-02-22T06:23:14Z

src/js/transport.js

-    rateLimitFn = (/^throttle$/i).test(o.rateLimitFn) ?
-      utils.throttle : utils.debounce;
+    maxConcurrentConnections = utils.isNumber(o.maxConcurrentConnections) ?
+      o.maxConcurrentConnections : maxConcurrentConnections || 6;


why not just have '6' instead of 'maxConcurrentConnections || 6', I don't see how maxConcurrentConnections could be instantiated at this point?

maxConcurrentConnections is a static variable shared between each Transport instance. So if you have the following code:

$('.typeahead-one').typeahead({ maxConcurrentConnections: 10 }); $('.typeahead-one').typeahead({ maxConcurrentConnections: 4 });

The final value for maxConcurrentConnections should be 4.

aha, makes sense. I was considering the single transport case.

One more thing we should discuss is if it makes sense to have the transports share a cache. Might make more sense to give each transport its own cache instance, reducing the coupling between the activity on the inputs.

jharding · 2013-02-22T07:35:09Z

One more thing we should discuss is if it makes sense to have the transports share a cache. Might make more sense to give each transport its own cache instance, reducing the coupling between the activity on the inputs.

I could go either way on this. @timtrueman any thoughts?

timtrueman · 2013-02-22T07:42:59Z

I'd do whatever is simplest, keeping in mind non-robots can only type into one input at a time and the likelihood of returning one you just used after switching to another seems low?

jharding · 2013-02-22T08:00:17Z

The more I think I about it, the more I favor having a shared request cache. If we have a request cache per transport, then the number of potentially cached requests grows with the number of datasets. So if you had 10 datasets, we could potentially be caching 100 (or more) requests and that's just overkill.

Also, the browser will most likely be caching these requests so it's fine if we're aggressive in our cache eviction policy.

timtrueman · 2013-02-22T08:03:38Z

Agreed. The core value of a request cache in my mind is for the delete key case.

vskarich · 2013-02-22T08:04:09Z

That sounds fair to me. I'm for leaving it as-is.

On Fri, Feb 22, 2013 at 12:00 AM, Jake Harding notifications@github.comwrote:

The more I think I about it, the more I favor having a shared request
cache. If we have a request cache per transport, then the number of
potentially cached requests grows with the number of datasets. So if you
had 10 datasets, we could potentially be caching 100 (or more) requests and
that's just overkill.

Also, the browser will most likely be caching these requests so it's fine
if we're aggressive in our cache eviction policy.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/25#issuecomment-13932592.

jharding · 2013-02-22T08:05:09Z

I'll update RequestCache to be a singleton tomorrow.

sullimander · 2013-02-22T17:28:01Z

Looks good to me. I like the idea of opening all the options to ajax. The one option that might be problematic though is dataType, since the rest of the library is build on the assumption that the response is JSON. Maybe whitelist it to just json and jsonp. Also there is no default set for dataType.

jharding · 2013-02-23T08:32:28Z

That's a good point, we should definitely whitelist json and jsonp.

dogoku · 2013-02-23T09:31:05Z

src/js/transport.js

@@ -53,29 +48,23 @@ var Transport = (function() {

      url = url.replace(this.wildcard, encodeURIComponent(query || ''));


i think this is not being used in the ajax call anymore, which i'm guessing is wrong?
it should overwrite the ajaxSettings.url (or something along those lines)

Good catch, I'll make sure to address this.

dogoku · 2013-02-23T09:32:16Z

I actually like this approach a lot better than mine. Having a Transport for each Dataset seems to work great

jharding · 2013-02-24T06:23:51Z

I'm questioning whether or not making every option configurable, it might be too overwhelming. Also, this wouldn't exactly expose a great way of processing responses before typeahead.js got a hold of it. You could use dataFilter, but since that gets passed the raw response, you'd have to parse it yourself. Also dataFilter wouldn't get called for jsonp requests since it's not a XHR.

As of right now, here's what I know we need to support:

jsonp
setting timeout
setting headers
turning off browser caching
response data filtering

If this is all we need to support, I say we add the following options:

dataType
timeout
beforeSend
cache
filter

The first 4 we would just pass along to jQuery.ajax, and the last one we would call immediately in the success callback.

Thoughts?

My proposal for the new API:

$('.typeahead').typeahead({
  name: 'states',
  limit: 5,
  template: '<p>{{value}} – United States of America</p>',
  engine: Hogan,
  local: ['Michigan', 'California'],
  prefetch: {
    url: '/states.json',
    ttl: 3600000,
    filter: function(resp) { /* parse suggestions out of response  */ }
  },
  remote: {
    url: '/states?q=%WILDCARD',
    wildcard: '%WILDCARD',
    rateLimitFn: 'throttle',
    rateLimitWait: 300, // rateLimitWait or wait? i think i prefer the former
    maxConcurrentConnections: 4,
    dataType: 'json',
    timeout: 2000,
    cache: true,
    beforeSend: function(jqXhr, settings) { /* set headers, etc. */ },
    filter: function(resp) { /* parse suggestions out of response  */ }
  }
});

dogoku · 2013-02-24T10:35:44Z

It's a shame dataFilter can't be used for jsonp

Conflicts: src/js/typeahead.js

jharding · 2013-02-26T03:45:04Z

I am also thinking we should add a replace option to the remote object to handle issues like #20.

…port Conflicts: src/js/typeahead.js

jharding · 2013-02-26T06:21:41Z

I'm really happy with the changes I made. It doesn't complicate the simple use case and it allows for maximum configuration for the advanced use cases. The following APIs would be supported with this pull request:

Simple

$('.typeahead').typeahead({
  name: 'books',
  local: ['Watership Down', 'Player Piano'],
  prefetch: '/top_books.json',
  remote: 'books?q=%QUERY'
});

Advanced

$('.typeahead').typeahead({
  name: 'books',
  template: '<h2>{{value}}</h2>',
  engine: Hogan,
  local: ['Watership Down', 'Player Piano'],
  prefetch: {
    url: '/top_books.json',
    ttl: 60 * 60 * 1000,
    filter: function(data) { /* filter data */ }
  },
  remote: {
    name: 'books?q=%BOOK',
    wildcard: '%BOOK',
    replace: function(url, uriEncodedQuery) { 
      // if set, wildcard is ignored and this function handles
     // query subsitution
    },
    rateLimitFn: 'throttle',
    rateLimitWait: 200,
    maxParallelRequests: 6,
    dataType: 'json',
    timeout: 2000,
    cache: true,
    beforeSend: function(jqXhr, settings) { /* set headers, etc. */ },
    filter: function(resp) { /* parse suggestions out of response  */ }    
});

timtrueman · 2013-02-26T06:38:25Z

I like it.

vskarich · 2013-02-26T06:55:14Z

+1

On Mon, Feb 25, 2013 at 10:38 PM, Tim Trueman notifications@github.comwrote:

I like it.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/25#issuecomment-14097939
.

jharding · 2013-03-04T05:15:16Z

Merged into integration-0.9.0.

stayce · 2013-04-10T23:35:20Z

Is JSONP supported? Can I make XHR request? Is there an example? Thanks very much

jharding · 2013-04-10T23:40:52Z

Check out the remote section of the README.

Is JSONP supported?

You should be able to use JSONP by setting dataType to jsonp. I haven't actually tested this though so let me know if you experience any problems.

$('.typeahead').typeahead({
  name: 'jsonpExample',
  remote: {
    // ...
    dataType: 'jsonp'
  }
});

Can I make XHR request?

Not really sure what you mean by this.

stayce · 2013-04-11T00:02:06Z

Thanks @jharding , jsonp seems to be working.

zemirco · 2013-08-07T13:06:48Z

jsonp doesn't work for me. See the example using the Bing Maps API for querying locations. I always get Uncaught SyntaxError: Unexpected token :

Test as much as you want. It's not a production key :)

zh99998 · 2013-09-18T13:26:25Z

seems there is no way to set jsonpCallback?

First pass at transport rewrite.

f125f57

vskarich reviewed Feb 22, 2013
View reviewed changes

Have transports share a request cache.

ceff358

This was referenced Feb 23, 2013

Add support for a custom remotePreprocessor function & autodetect JSONP #29

Closed

Add support for jsonp remote requests #13

Closed

dogoku reviewed Feb 23, 2013
View reviewed changes

jharding mentioned this pull request Feb 26, 2013

Xss issue #18

Closed

Merge remote-tracking branch 'upstream/master' into rewrite-transport

a6b249a

Conflicts: src/js/typeahead.js

Jake Harding added 3 commits February 25, 2013 21:02

Add support for new API.

fd397ab

Merge branch '42-multiple-elements-initializaiton' into rewrite-trans…

68266d6

…port Conflicts: src/js/typeahead.js

Support original API.

bdb56e3

Jake Harding added 3 commits February 26, 2013 18:48

Add request cache mock for tests.

7632109

Add tests.

6674bd1

Work on test suite.

77656a5

jharding mentioned this pull request Mar 4, 2013

v0.9.0 #80

Merged

jharding closed this Mar 4, 2013

zemirco mentioned this pull request Aug 27, 2013

allow setting jsonp option for remote data #417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite transport #25

Rewrite transport #25

jharding commented Feb 22, 2013

vskarich Feb 22, 2013

jharding Feb 22, 2013

vskarich Feb 22, 2013

vskarich Feb 22, 2013

jharding commented Feb 22, 2013

timtrueman commented Feb 22, 2013

jharding commented Feb 22, 2013

timtrueman commented Feb 22, 2013

vskarich commented Feb 22, 2013

jharding commented Feb 22, 2013

sullimander commented Feb 22, 2013

jharding commented Feb 23, 2013

dogoku Feb 23, 2013

jharding Feb 24, 2013

dogoku commented Feb 23, 2013

jharding commented Feb 24, 2013

dogoku commented Feb 24, 2013

jharding commented Feb 26, 2013

jharding commented Feb 26, 2013

timtrueman commented Feb 26, 2013

vskarich commented Feb 26, 2013

jharding commented Mar 4, 2013

stayce commented Apr 10, 2013

jharding commented Apr 10, 2013

stayce commented Apr 11, 2013

zemirco commented Aug 7, 2013

zh99998 commented Sep 18, 2013

		@@ -53,29 +48,23 @@ var Transport = (function() {

		url = url.replace(this.wildcard, encodeURIComponent(query \|\| ''));

Rewrite transport #25

Rewrite transport #25

Conversation

jharding commented Feb 22, 2013

vskarich Feb 22, 2013

Choose a reason for hiding this comment

jharding Feb 22, 2013

Choose a reason for hiding this comment

vskarich Feb 22, 2013

Choose a reason for hiding this comment

vskarich Feb 22, 2013

Choose a reason for hiding this comment

jharding commented Feb 22, 2013

timtrueman commented Feb 22, 2013

jharding commented Feb 22, 2013

timtrueman commented Feb 22, 2013

vskarich commented Feb 22, 2013

jharding commented Feb 22, 2013

sullimander commented Feb 22, 2013

jharding commented Feb 23, 2013

dogoku Feb 23, 2013

Choose a reason for hiding this comment

jharding Feb 24, 2013

Choose a reason for hiding this comment

dogoku commented Feb 23, 2013

jharding commented Feb 24, 2013

dogoku commented Feb 24, 2013

jharding commented Feb 26, 2013

jharding commented Feb 26, 2013

timtrueman commented Feb 26, 2013

vskarich commented Feb 26, 2013

jharding commented Mar 4, 2013

stayce commented Apr 10, 2013

jharding commented Apr 10, 2013

stayce commented Apr 11, 2013

zemirco commented Aug 7, 2013

zh99998 commented Sep 18, 2013