Skip to content

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

Merged
capricorn86 merged 4 commits into
capricorn86:masterfrom
odanado:fix/1308
Mar 14, 2024
Merged

fix: [#1308] Fix a bug of multiple fallbacks of custom property#1309
capricorn86 merged 4 commits into
capricorn86:masterfrom
odanado:fix/1308

Conversation

@odanado

@odanado odanado commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

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

@odanado
odanado requested a review from capricorn86 as a code owner March 14, 2024 17:42

@capricorn86 capricorn86 left a comment

Copy link
Copy Markdown
Owner

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
@capricorn86

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