Improve build/generate-sri.js regex

[Noah2610]

When using single-quotes in config.yml, the previous
regular expression in build/generate-cli.js wasn't working correctly,
it was replacing ALL string values with hashes.
Now both double- and single-quotes can be used in config.yml,
and the RegExp will work as expected.

---

I have tested the script with both ' and " used in config.yml, and it seems to work properly!
The changes to config.yml haven't been committed, though.

Fixes #29741

[Noah2610]

Sorry, my bad! I forgot to run npm run test! Thanks for fixing my mistake...

[XhmikosR]

@Noah2610 please remove any dist files :)

[Noah2610]

@XhmikosR Remove the dist files from my fork? Or where do you mean?

[XhmikosR]

From your branch. No dist files should be touched.

[XhmikosR]

Do not delete the dist files... just don't touch them in your patch

[Noah2610]

I'm sorry, I'm not following... I haven't touched the dist/ files in my original commit. I'm sorry for any trouble I'm causing! Would you mind explaining to me what I'm doing wrong?

[XhmikosR]

You deleted the dist files when you shouldn't have touched them in the first place. I fixed it, please don't overwrite the changes.

[mdo]

Up to you whether we keep this @XhmikosR.

[XhmikosR]

Still need to test this out, I forgot it even existed :P

[mdo]

I mean I'm fine leaving it as-is given it works for us. Still your call.

[XhmikosR]

@Noah2610 can you please make a https://regex101.com/ example? We don't really hit this issue, but if it helps other people we can consider merging it.

PS. any reason you don't use non-capturing groups?

[Noah2610]

@XhmikosR Here's the regex101: https://regex101.com/r/Yhl9vL/1
Note, that the \w+_hash part in the regex is only for regex101, in the generate-sri.js script it is replaced with the property name.
Also note, that some values in the regex101 string example use " or ' quotes, to showcase both cases.

I don't remember why I went so heavy on the capturing-groups. I cleaned it up a bit in the last commit.

[XhmikosR]

Perfect, thanks!

I can quickly confirm this is fine since the Netlify preview doesn't throw any SRI mismatch errors, but I'll test it later more and merge it soon. I'll also backport this to v4-dev if/after this lands.

[XhmikosR]

One last thing: any reason you switched to * from +? I'm not sure this change is needed.

[Noah2610]

Did I? I don't see any + in the regex in the old commit.

[XhmikosR]

s+, \S+.

We might as well specify m and use $ since you added ^.

EDIT: scratch the comment about m, forgot for a moment we are using sed there :)

[Noah2610]

Oh sorry, I see now.
I think I did that originally, because technically it's still correct yaml, with no whitespace after the : (if I'm not mistaken).
So this is still valid yaml: propertyName:"propertyValue".
EDIT: I am mistaken, it is invalid yaml.

And yeah, I'll add the $.

[XhmikosR]

I edited directly the file, let me know if everything still looks good to you. If you can, please also update your regex101 example.

[Noah2610]

https://regex101.com/r/Yhl9vL/2/

[XhmikosR]

Perfect, I think everything's is fine, but I'll test later again.

Thanks!

[Noah2610]

https://regex101.com/r/Yhl9vL/3

I added a badly formatted yaml line to the end of the example string to showcase why \s* may be better than \s+.
But it's your call, should work for basically all cases anyway.

EDIT: Although the whitespace at the start of the line is invalid yaml in this case, so the first \+ should definitely stay.

[XhmikosR]

So, basically, only the second \s+ is valid, right? If so, feel free to push the change and update the regex101 link one last time :)

[Noah2610]

The first \s+ is valid (to match with whitespace at the start of the line),
The second \s+ should be * to match the case where there's no whitespace between the : and the value.
And the third \S+ (non-whitespace) should probably also be * to allow for empty hash values by default, for example if the user deletes all hashes (empties the strings) before they run the script.

Another thing I noticed, is that we probably shouldn't have a $ at the end, to allow for trailing whitespace or comments at the end of the line.

Here's the updated regex101: https://regex101.com/r/Yhl9vL/4

I'll update the script and test it locally before I commit.

[XhmikosR]

Well, TBH, I don't think we should cover all those cases. The script is supposed to be used internally, and it's OK to make a small change to take care of your issue, but I don't think we should care about all the other cases.

[Noah2610]

I understand.
I have the updated regex, let me know if you want me to commit, or if we should just leave it as is.

[Noah2610]

Also, nevermind, yaml requires a space after the :, I was wrong about that being valid yaml.
https://stackoverflow.com/questions/42124227/why-does-the-yaml-spec-mandate-a-space-after-the-colon
https://regex101.com/r/Yhl9vL/5

[XhmikosR]

OK, so what's your final suggestion?

[Noah2610]

I'd use the regex in the last version of the regex101: https://regex101.com/r/Yhl9vL/5
It just changes the \S+ to \S* and removes the $.
Doesn't break anything and it isn't a big regex change. I can commit it right away.

[XhmikosR]

Cool, let's go with that then 👍