Generate HTML4 compliant ids

[timroes]

Partly adresses #635 by always generating ids, that begin with the letter i before 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.

[pugnascotia]

The problem I had was with src/components/form/form_row/make_id.js, so this PR as-is wouldn't have fixed my problem. Also, we now seem to have two modules performing similar roles, unless I'm missing something? Also, generating potentially two UUIDs for each ID generated feels a little like overkill.

Paging @cjcenizal for his opinion.

[timroes]

Hmm yeah, I also think there shouldn't be multiple generator implementations, but let's wait for CJ's input there :)

[cjcenizal]

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 makeId with htmlIdGenerator and strive for a single solution. Here's my train of thought:

• To @pugnascotia's point about generating uuids, can we run some tests to verify whether this has a significant performance cost? Massive forms are generally a poor UX, so I think even in an extreme case we probably won't be calling it more than 100 times in a single frame (warning: number pulled out of thin air).
• If there are no performance problems then I think we can replace makeId with htmlIdGenerator.
• If there are problems then maybe makeId is more performant and we can update htmlIdGenerator to use it internally?

[pugnascotia]

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:

export function htmlIdGenerator(idPrefix = `id-`) {
  return (suffix = ``) => `${prefix}${uuid.v1()}${suffix}`;
}

[timroes]

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 :)

[cjcenizal]

I created #639 to capture the need for better documentation for htmlIdGenerator and #640 to capture my suggestion for an id propType checker.

[pugnascotia]

@timroes just noticed this PR was still open - what do you wan to do with it?

[timroes]

Good question. I think we could still merge this, and have the makeId generator return HTML 4 compliant IDs. Though this would not solve your issue, if you are still using the internal id generator of form rows. Nevertheless I think we should decouple the cleanup of different generators into other PRs. So if you don't mind I would appreciate a review :-)

[cjcenizal]

I won't have time to review this unfortunately, so I'm removing myself and adding @chandlerprall.

[chandlerprall]

Needs a changelog entry, otherwise code change & test looks good.

[chandlerprall]

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.