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

fix: [#1308] Fix a bug of multiple fallbacks of custom property #1309

Merged
merged 4 commits into from Mar 14, 2024

Conversation

odanado
Copy link
Contributor

@odanado odanado commented Mar 14, 2024

Fix: #1308

The previous code was incorrect in its use of RegExp.prototype.exec, and the regular expression matching was performed only once.

RegExp.prototype.exec is difficult to use correctly because it is a stateful API.
I modified the logic using String.prototype.matchAll as described in MDN.

In addition, String.prototype.matchAll() helps to simplify matching multiple parts of a string (with capture groups) by allowing you to iterate over the matches.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/exec#description

Copy link
Owner

@capricorn86 capricorn86 left a comment

Choose a reason for hiding this comment

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

Thank you for contributing @odanado! 🌟

matchAll() will execute a find all and return an iterator for each loop in the while statement, which will not have very good performance.

I instead corrected the exec() to run on the modified value, which solved the problem.

@capricorn86 capricorn86 merged commit 8786172 into capricorn86:master Mar 14, 2024
3 checks passed
@capricorn86
Copy link
Owner

Sorry @odanado, I was a bit fast. exec() won't work, so I changed it to this implementation using match() instead (which should also have good performance):
#1312

Not sure why the unit tests passed in the PR 🤔

@odanado odanado deleted the fix/1308 branch March 15, 2024 01:48
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.

If multiple custom property fallbacks occur, getPropertyValue returns an empty string
2 participants