fmoo / react-typeahead

Pure react-based typeahead and typeahead-tokenizer
ISC License
677 stars 231 forks source link

Fix issue 128 where the autocomplete was one step behind the input #146

Open alexjg opened 8 years ago

alexjg commented 8 years ago

This fixes #128 where the autocomplete was one step behind the input due to a componentWillReceiveProps handler overwriting the entryValue. The fix has been to take Typeahead.state.visible and make it a method call so it can be calculated based on the state at render time.

fmoo commented 8 years ago

This looks good overall. Can you run npm run benchmark before and after so we can know how if we're looking at a significant performance regression as a result?

cc @growlsworth

alexjg commented 8 years ago

I've updated the name and fixed the whitespace issue. I can't see any script for benchmarking, either I'm missing something or it needs to be set up. I'm happy to set something up, especially if anyone has an example of a benchmarking setup on a project like this for me to shamelessly copy.

trshafer commented 8 years ago

I thought I still had my benchmark for https://github.com/fmoo/react-typeahead/pull/125 . I can recreate it and send it here.

trshafer commented 8 years ago

npm install benchmark

And here's a benchmark shell with an initialized typeahead.

var Benchmark = require('benchmark');
var React = require('react');

var ReactTypeahead = require('../lib/react-typeahead').Typeahead;
var TypeaheadElement = React.createElement(ReactTypeahead);
var props = {
  options: [],
};
var typeaheadInstance = new TypeaheadElement.type(props);

console.log("running benchmark");

// example from: http://benchmarkjs.com/
var suite = new Benchmark.Suite();
suite.add('Run 1', function() {
  typeaheadInstance.getInitialState()
})
// .add('Run 2', function() {
// })
// .add('Run 3', function() {
// })
.on('complete', function() {
  console.log('Fastest is ' + this.filter('fastest').pluck('name'));
})
.run();

console.log("done");
alexjg commented 8 years ago

Have added a simple benchmark which renders the component into a jsdom document, enters characters and asserts the typeahead options showing. With both this code and master the mean runtime is 0.005.

The benchmark can be run with npm benchmark. At the moment it just spits out the mean runtime, obviously there are more interesting things which could be done but just in the interest of getting this PR merged I've not done anything more.

alexjg commented 8 years ago

@fmoo @growlsworth Has either of you had a chance to review this?

trshafer commented 8 years ago

Apologies @alexjg. didn't know you were looking for feedback. I'm in family vacation at the moment. Will check back in a week. If you don't hear back ping me again.

fmoo commented 8 years ago

Bump on this all.

I ended up merging in #163 which solved this, but broke backwards compatibility.

I'm reverting that and wondering if this would be a better fix for 1.1.x.? Otherwise, I'll remerge #163 and #167 as 2.0.x

cc @nosilleg , @jeffsaracco

fmoo commented 8 years ago

Hey @thomasjshafer , @alexjg . I recently merged in #163 which did something similar to this, but broke backwards compatibility in 1.1.6.

I'm going to revert it for 1.1.7, and was going to reapply along with #167 for a 2.0.0(-beta1?) release, but wondering if you had any thoughts on how that approach compares to this one.

cc @nosilleg for his thoughts as well.