-
Notifications
You must be signed in to change notification settings - Fork 429
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
Better Regex to ignore $this variables in the code #134
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/builder-io/partytown/CHe6saxhRKV5qro47RApC2WZFEJ6 |
Doesn't look like it's passing webkit. @jesse-deboer would you be able to see what the issue is? Thanks |
Hi @adamdbradley, this was already broken as you reported in this issue #131. I will try to find out why it's not working, but it's not because of this PR I think .. |
It looks like this error is only for webkit that every test fails, which is different from the other issue (i think). |
Sorry but this doesn't work on Safari. When I run it locally Safari will error with |
@adamdbradley you are indeed right, Safari unfortunately still doesn't support look-behind. I'll try and make this RegEx so that it will also support Safari! |
@jesse-deboer can you test below
Explanation for above codeLink: MDN docs for String::replace Instead of using Look-behind to check for $, let’s do manual checking based on match substring. first let’s revert to previous regex. // look behind regex
scriptContent.replace(/(?<!\$)\bthis\b/g, '(thi$(this)?window:this)').replace(/\/\/# so/g, '//Xso')
// previous regex
scriptContent.replace(/\bthis\b/g, '(thi$(this)?window:this)').replace(/\/\/# so/g, '//Xso')
// more specifically first replace function
scriptContent.replace(/\bthis\b/g, '(thi$(this)?window:this)') Now this regex will match all this (ie. same code as above with second argument as function // return value will be used as replacement value
scriptContent.replace(/\bthis\b/g, () => {
return '(thi$(this)?window:this)';
}) The replacement function accepts few arguments. check the docs for more info about arguments. function replacer(match, p1, p2, /* …, */ pN, offset, string, groups) {
return replacement;
} look at below example, offset will always gives us the index of matched substring. so using offset we can check if previous index is “$” or not (i.e. offset - 1). // there are no capture groups, so no need to accept p1,...pn args
scriptContent.replace(/\bthis\b/g, (match, offset, originalStr) => {
if (offset > 0 && originalStr[offset - 1] !== "$") {
return '(thi$(this)?window:this)';
}
}) As per our regex we are matching all the substring with “this” and inside the function we are only returning the replacement value if we don’t find the “$” before matched substring but all other time we are returning “undefined”. To take care of this we’ll just have to return matched substring as it is. scriptContent.replace(/\bthis\b/g, (match, offset, originalStr) => {
if (offset > 0 && originalStr[offset - 1] !== "$") {
return '(thi$(this)?window:this)';
}
// otherwise just return match character.
return match;
})
// one liner with ternary operator
scriptContent.replace(/\bthis\b/g, (match, offset, originalStr) => (offset > 0 && originalStr[offset - 1] !== "$") ? '(thi$(this)?window:this)' : match); |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Great solution, let's see if it works, just committed the code to the patch! |
@adamdbradley what's the status on this? |
Co-authored-by: Adam Bradley <[email protected]>
At this moment the regex matches with the word
this
but also with$this
, that should not be the case. This way$this
is ignored from being replaced, this is used in some 3rd party scripts.