Conversation
|
The problem I had was with Paging @cjcenizal for his opinion. |
|
Hmm yeah, I also think there shouldn't be multiple generator implementations, but let's wait for CJ's input there :) |
|
I think prefixing IDs with a letter is a good solution. We may want to create a propType checker which will throw an error if the ID doesn't begin with a letter, to help people learn about and embrace best practices. But this can be done in another PR. I agree we should try to reconcile
|
|
I most just find it surprising that the generator constructs two UUIDs if neither a prefix nor suffix is supplied, since a UUID is already supposed to be pretty unique. Why not just do: |
|
Because the id generator needs to create reproducable IDs when using the same instance. const generator1 = htmlIdGenerator();
const generator2 = htmlIdGenerator();
generator1('foo') === generator1('foo');
generator1('foo') !== generator2('foo');
generator2('foo') === generator2('foo');
generator1('foo') !== generator1('bar');Thus for the same specified suffix we always need to generate the same id on one instance of the generator, but another than on another instance. That's why if you don't specify a prefix, we will generate one. Whether or not you now specify a suffix, is up to your usecase and in case you don't want determenistic IDs and thus not specify a suffix, we generate a suffix for you. The example you have given, would never generate deterministic IDs, that you could regenerate at another place, because each call would have it's own UUID in it. Also there is not much of a performance drawback, since the prefix UUID will just generated once when the component is created, so that shouldn't be too much of an overhead. If you would worry about the overhead, you can still pass in your custom prefix, and are down to one UUID generation :) |
|
@timroes just noticed this PR was still open - what do you wan to do with it? |
|
Good question. I think we could still merge this, and have the |
|
I won't have time to review this unfortunately, so I'm removing myself and adding @chandlerprall. |
chandlerprall
left a comment
There was a problem hiding this comment.
Needs a changelog entry, otherwise code change & test looks good.
chandlerprall
left a comment
There was a problem hiding this comment.
LGTM; @timroes please remove the "Fixes" statement in the description as it does not directly address Rory's case and shouldn't auto-close that issue.
Partly adresses #635 by always generating ids, that begin with the letter
ibefore the random generated prefix. That way we won't end up with ids beginning with a number.If the user specifies a prefix, we won't prefix the
i, but expect the user to specify a prefix, that will begin with a letter instead to stay compliant.