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

escape regex special characters #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

escape regex special characters #20

wants to merge 1 commit into from

Conversation

kruczy
Copy link
Contributor

@kruczy kruczy commented Feb 10, 2016

Escape regexp special characters to prevent "invalid regexp" errors and unexpected behaviour while querying for candidates

@therebelrobot
Copy link
Collaborator

Initial look LGTM. I'll pull your changes and see how they hold up under some tests (which reminds me, we need to get tests in here...), and will check back in tonight most likely.

@therebelrobot therebelrobot mentioned this pull request Feb 10, 2016
@kruczy
Copy link
Contributor Author

kruczy commented Feb 10, 2016

Thanks for quick reply, you can try something like "c++" "test(" or any combination of regexp special chars that will not create a valid one

could also move the regexp to a static so browser wont have to do so much work

@therebelrobot
Copy link
Collaborator

Will definitely test those, I've also got a few more I can try. Will you add as many test cases as you can think of over on #21? I want to make sure I'm hitting as many as possible when building out tests.

could also move the regexp to a static so browser wont have to do so much work

Could you elaborate more on this? This lib needs to be able to be run entirely browser-side, so I'm not sure how you'd unload some of that off. (maybe I'm just misunderstanding?)

@kruczy
Copy link
Contributor Author

kruczy commented Feb 10, 2016

what I'm saying is quite simple, just TypAhead.FILTER_SANITIZE_REGEXP = /..../ and use that later

I guess browsers can optimize for inline regex use tho, so should not make much difference

testing on IE 11, Edge, Chrome and FF seems only IE would benefit from this in a meaningful way, probably should just leave it as is for readibility

@therebelrobot
Copy link
Collaborator

If you'd like to move it to a separate declaration, feel free. So long as it works, I don't have a preference.

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.

2 participants