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

Better Regex to ignore $this variables in the code #134

Merged
merged 5 commits into from
Mar 21, 2023

Conversation

jesse-deboer
Copy link
Contributor

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.

@vercel
Copy link

vercel bot commented Mar 25, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/builder-io/partytown/CHe6saxhRKV5qro47RApC2WZFEJ6
✅ Preview: https://partytown-git-fork-jesse-deboer-patch-1-builder-io.vercel.app

@adamdbradley
Copy link
Contributor

Doesn't look like it's passing webkit. @jesse-deboer would you be able to see what the issue is? Thanks

@jesse-deboer
Copy link
Contributor Author

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 ..

@adamdbradley
Copy link
Contributor

It looks like this error is only for webkit that every test fails, which is different from the other issue (i think).

@adamdbradley
Copy link
Contributor

Sorry but this doesn't work on Safari. When I run it locally Safari will error with SyntaxError: Invalid regular expression: invalid group specifier name.

@adamdbradley adamdbradley added the help wanted Extra attention is needed label Jun 1, 2022
@jesse-deboer
Copy link
Contributor Author

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

@nilesh-maurya
Copy link

@jesse-deboer can you test below replace function, i think it should work for Safari also.

scriptContent.replace(/\bthis\b/g, (match, offset, originalStr) =>  (offset > 0 && originalStr[offset - 1] !== "$") ? '(thi$(this)?window:this)' : match);
Explanation for above code

Link: MDN docs for String::replace

Untitled

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. this and $this). As per MDN docs second argument of string::replace function can be either string or function.

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.
example

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);

@vercel
Copy link

vercel bot commented Sep 10, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
partytown ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 21, 2023 at 3:46PM (UTC)

@jesse-deboer
Copy link
Contributor Author

@jesse-deboer can you test below replace function, i think it should work for Safari also.

scriptContent.replace(/\bthis\b/g, (match, offset, originalStr) =>  (offset > 0 && originalStr[offset - 1] !== "$") ? '(thi$(this)?window:this)' : match);

Explanation for above code

Great solution, let's see if it works, just committed the code to the patch!

@peterjaap
Copy link

@adamdbradley what's the status on this?

@adamdbradley adamdbradley merged commit 5f69f2f into BuilderIO:main Mar 21, 2023
vinaygosain pushed a commit to vinaygosain/partytown that referenced this pull request Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants