homerchen19 / github-file-icons

🌈 🗂 A browser extension which gives different filetypes different icons to GitHub, GitLab, gitea and gogs.
https://chrome.google.com/webstore/detail/file-icon-for-github-and/ficfmibkjjnpogdcfhfokmihanoldbfe
MIT License
1.43k stars 69 forks source link

Bug in extension for Safari - regex in icondb.js #123

Closed christopherpickering closed 2 years ago

christopherpickering commented 2 years ago

Hey, There is a regex issue in the new build for Safari.

I'm getting an error on this line (2614?) in content.bundle.js.

image

(I just ran a js formatter on the js so I could see what line was causing the issue)

["tag-icon", ["medium-green", "medium-green"], /(?:\.|^)sha(?:256|sum)?$|(?:\.|^)(?:check|ck|crc(?:32)?|md5|rmd160|sha(?:224|256|384|512|1|2|3)?)?(?:sums?|(?<=\w))$/i, , !1, , /\.text\.checksums$/i],

If I comment out this line then everything is working ok.

@csandman do you have any ideas? Can we just drop that line when doing update assets?

Its coming in from file-icons/db/icondb.js.

I'm kinda surprised this isn't popping up in chrome? I'm not sure how to test there.

To reproduce in Safari:

npm install
npm run update-assets
npm run build

Xcode, open /safari/File Icons for GitHub and GitLab.xcodeproj

Change to MacOS: image

iRoachie commented 2 years ago

Can confirm, the extension does not work in safari.

christopherpickering commented 2 years ago

@iRoachie the App Store is publishing a working update w/out that line, they seem to release them around 5pm cst.. so 6 hrs from now 👍🏽

csandman commented 2 years ago

I checked this out, it looks like it's breaking because the line includes a positive lookbehind, a feature that isn't supported in Safari or Firefox. You can see the warning here: https://regexr.com/6oovg

I can submit a PR to remove this part in the asset updating script. I'll do it for just this line, if nothing else is broken, but if there is anything else let me know!

christopherpickering commented 2 years ago

Thanks for checking it! Since it is not inside a group, would it be safe to just replace the group w/ \w+?

(?:\.|^)sha(?:256|sum)?$|(?:\.|^)(?:check|ck|crc(?:32)?|md5|rmd160|sha(?:224|256|384|512|1|2|3)?)?(?:sums?|w+)$
christopherpickering commented 2 years ago

Looks like you already got it covered in your pr :) I'm sure removing it is fine too. It seems like an extremely edgecasy regex.

csandman commented 2 years ago

Actually, you're right, I got the purpose of a positive lookahead mixed up for a second. Positive lookarounds are only really useful when you care about the captured content, where in our case we're just using the RegEx to test another string. I'm going to tweak my PR to replace any positive lookarounds with their content.

RegEx to replace RegEx with other RegEx is wild looking haha.

christopherpickering commented 2 years ago

@iRoachie Hey, the App Store just released... working for me :)

iRoachie commented 2 years ago

Just got it! Works great! Thanks so much 🚀