[BUGFIX LTS] unique-id: Ensure return value starts with a letter to avoid invalid selectors starting with numeric digits

[Turbo87]

This should fix #19882 (comment), by adding a prefix to IDs produced by the unique-id helper. While getElementById() works fine with IDs that start with numeric digits, the querySelector() functions do not accept them. UUIDs, by definition, are allowed to start with numbers which is causing these issues.

[chancancode]

I think the comment below is suggesting that we specifically do not want to do this. In the past, things like id="ember-*" became de-facto relied upon and impossible to change. The proposed change has a similar issue in that people could come to rely on the id having a particular format (starting with em-).

The original authors of the code chose to use a UUID v4 here to avoid this problem. Arguably, the UUID v4 format itself – xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx – could become relied upon, though it's much harder than a fixed prefix. Perhaps we should have made it all random characters, but perhaps there were more considerations that went into it and in any case that's probably out of scope for what you are trying to do/fix here.

Instead, I figured we could have preserved the UUID v4-ness and still resolve the issue you raised here, with this One Magical Tweak™:

 function uniqueId() {
   // @ts-expect-error this one-liner abuses weird JavaScript semantics that
   // TypeScript (legitimately) doesn't like, but they're nonetheless valid and
   // specced.
-   return ([1e7] + -1e3 + -4e3 + -8e3 + -1e11).replace(/[018]/g, (a) =>
-    (a ^ ((Math.random() * 16) >> (a / 4))).toString(16)
+   return ([3e7] + -1e3 + -4e3 + -2e3 + -1e11).replace(/[0-3]/g, (a) =>
+    ((a * 4) ^ ((Math.random() * 16) >> (a & 2))).toString(16)
   );
 }

Essentially, this generates a valid UUID v4, but fixes the first character to one of c/d/e/f. Since it always starts with a "letter" it should accomplish the same thing while not introducing a fixed prefix, respecting the original authors' wishes.

If anyone is interested I can unpack how it works 😬, but it requires first unpacking what the original version was trying to do and tweaking it slightly. But it basically does the same thing, just also restricting the first character.

We should probably add a test for this (asserting that the result can be a valid selector). It's a bit challenging without exposing the guts of this, but I figured calling the helper/running the assertions say 1000 times in a test is probably good enough proof that it's working?

[chancancode]

see above

[Turbo87]

@chancancode thanks for the review and feedback. I've adjusted the code as suggested and added an additional test case :)

[chancancode]

Ah. I mean that we should invoke the {{unique-id}} helper 1000 times in the test (since it includes some randomness, the iterations increase the chances of actually catching the bugs/confirming the behavior).

The easiest way to do it is probably generate a template with 1000 invocations, each surrounded by some kind of marker (eg [{{unique-id}}] [{{unique-id}}] …) then we can assert that each of the 1000 instances (which would be all different) all are valid selectors. Does that make sense?

[Turbo87]

The easiest way to do it is probably generate a template with 1000 invocations, each surrounded by some kind of marker (eg [{{unique-id}}] [{{unique-id}}] …) then we can assert that each of the 1000 instances (which would be all different) all are valid selectors. Does that make sense?

I might be misunderstanding but isn't that pretty much exactly what I did here? 😅

[chancancode]

You’re right, not sure what I read 😅 can you fix the lint? Then I think we are good to go

[chancancode]

Thank you!!