react-typeahead icon indicating copy to clipboard operation
react-typeahead copied to clipboard

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

Open alexjg opened this issue 9 years ago • 9 comments

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.

alexjg avatar Dec 07 '15 18:12 alexjg

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

fmoo avatar Dec 09 '15 16:12 fmoo

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.

alexjg avatar Dec 10 '15 17:12 alexjg

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 avatar Dec 10 '15 18:12 trshafer

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");

trshafer avatar Dec 10 '15 18:12 trshafer

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 avatar Dec 14 '15 19:12 alexjg

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

alexjg avatar Dec 22 '15 00:12 alexjg

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.

trshafer avatar Dec 22 '15 18:12 trshafer

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 avatar Mar 25 '16 00:03 fmoo

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.

fmoo avatar Mar 25 '16 00:03 fmoo