Skip to content

Conversation

alexjg
Copy link

@alexjg alexjg commented Dec 7, 2015

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Should be getVisibleOptions() for consistency with other accessors ?

Also, whitespace between () and {

@fmoo
Copy link
Owner

fmoo commented Dec 9, 2015

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
Copy link
Author

alexjg commented Dec 10, 2015

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
Copy link
Collaborator

I thought I still had my benchmark for #125 . I can recreate it and send it here.

@trshafer
Copy link
Collaborator

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
Copy link
Author

alexjg commented Dec 14, 2015

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
Copy link
Author

alexjg commented Dec 22, 2015

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

@trshafer
Copy link
Collaborator

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
Copy link
Owner

fmoo commented Mar 25, 2016

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
Copy link
Owner

fmoo commented Mar 25, 2016

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 fmoo mentioned this pull request Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Option search is being run one change behind

3 participants