-
Notifications
You must be signed in to change notification settings - Fork 164
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
Fix random bug where regex doesn’t match every reg. expression #47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too faced this same annoying error and came upon the same fix, since RegExp.exec mutates the RegExp object
In the meanwhile, while people are waiting for a fix, you can simply always ensure the pattern is a fresh instance in your render method. const HASHTAG_REGEX = /#[a-z]+\b/i
...
{ pattern: new RegExp(HASHTAG_REGEX) } |
@christophby & @scottmas I'd be happy to ship this if the suggested change was made, and if we add tests that exercise this bug, so we make sure not to have a regression. Please merge in master before you do that as I've switched the test harness to jest. 🥂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply:
https://github.com/taskrabbit/react-native-parsed-text/pull/47/files#r210374952
And please add tests!
Just tried this change locally and seems to be working as expected. Is it possible to merge this PR please? |
@shramadani -- if you want to help contribute tests for this PR, I'm looking to ship a new release of this this week. Please let me know if you're interested! |
@fbartho any updates on this? 😃 |
Since there are no current tests, I'm going to try to write some myself. Additionally, there was a comment that I requested get applied, and that was resolved (hidden) instead of applying it. I probably will reset the regexp by setting This means I'm probably not going to merge this PR as is. I hope you understand @christophby |
Alternative implementation to #47
There is an alternative implementation in this commit: e8f140e I will spin a beta build shortly |
|
Closing this PR as there would now be conflicts, but please report if you have any issues with the current implementation, or if further fixes are necessary! |
Im my case it skipped random matches. This PR fixes it.