Skip to content
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

A more performant updateOriginalInput #1460

Conversation

deefour
Copy link
Contributor

@deefour deefour commented Nov 16, 2018

See #910 for context.

This is a new PR rebasing the last 3 years of changes in master.

Previously, on every call to updateOriginalInput(), a brand new array of
options for every value in selectize.items would be created and the
entire HTML string would replace all children of the raw input source.

When doing something like

selectize.addItems(_.keys(selectize.options));

for a few hundred options, this becomes very sluggish.

This takes a smarter approach, only removing and adding individual
options from the input source as necessary.

The bottleneck is the repeated use of jQuery's html() method with large
DOM updates. For ~300 options this was originally taking 30154ms in
appendChild. Now it spends only 330.9ms.
@Pictor13
Copy link
Contributor

I didn't review the code but the change looks interesting and useful. Good job 👍🏻

As you can there are errors with Travis CI.
In order to submit a pull request correctly, just the source code under src/ directory should be modified, leaving untouched the code under dist/ (distributed build).
This latter will be automatically updated with all the latest pull-requests, once there will be a new build release.

@deefour
Copy link
Contributor Author

deefour commented Jun 18, 2019

I don't plan on putting any more work into this.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2021

Stale pull request message

@deefour
Copy link
Contributor Author

deefour commented Jan 4, 2021

It would be a shame for this PR to get auto-closed out by a Github action. While I cannot comment on a recent version of selectize, at the time the code in this PR was the difference between the dropdown box being unusable vs very performant.

As I mentioned 1.5 years ago, I won't be putting more work into this, but if it hasn't otherwise been fixed it'd be a shame not to not see the performance boost make it into a release.

@risadams risadams added the pending review This issue was closed as stale; since then additional review has been requested. label Jan 5, 2021
@risadams
Copy link
Contributor

Update on this PR, I haven't merged it yet because it is causing tests to fail; however I'm not particularly sure that the original code is working quite correctly, so until I can determine what the correct behavior is, this is going to sit in the pending queue for a bit

@marianoesteban
Copy link

Escaping value and label with escape_html() in line 1742 makes the tests pass:

fresh.push('<option value="' + escape_html(value) + '" selected="selected">' + escape_html(label) + '</option>');

@risadams
Copy link
Contributor

risadams commented Nov 4, 2022

Rebased, updated and merged in new PR #1895

@risadams risadams closed this Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending review This issue was closed as stale; since then additional review has been requested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants