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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Jun 21, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/@ember/-internals/glimmer/lib/helpers/unique-id.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@ export default internalHelper((): Reference<string> => {
// This code should be reasonably fast, and provide a unique value every time
// it's called, which is what we need here. It produces a string formatted as a
// standard UUID, which avoids accidentally turning Ember-specific
// implementation details into an intimate API.
// implementation details into an intimate API. It also ensures that the UUID
// always starts with a letter, to avoid creating invalid IDs with a numeric
// digit at the start.
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)
);
}
18 changes: 18 additions & 0 deletions packages/@ember/-internals/glimmer/tests/integration/helpers/unique-id-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,24 @@ if (EMBER_UNIQUE_ID_HELPER) {
});
}

['@test it only generates valid selectors']() {
let iterations = 1000;
let reNumericStart = /^\d/;

let template = '<p>{{unique-id}}</p>'.repeat(iterations);
super.render(template);

for (let i = 0; i < iterations; i++) {
let textNode = this.nthChild(i).firstChild;
let text = textNode.data;

this.assert.false(
reNumericStart.test(text),
`{{unique-id}} should produce valid selectors` + text
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

render(template, ...rest) {
// If there are three parameters to `render`, the second parameter is the
// template's arguments.
Expand Down