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.

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

Fast React ID Generator #528

Merged
merged 3 commits into from Apr 12, 2018

Conversation

Projects
None yet
3 participants
@DaniilSokolyuk
Contributor

DaniilSokolyuk commented Apr 8, 2018

Fast React ID Generator

@Daniel15

This seems OK to me. @dustinsoftware what do you think?

var id = Interlocked.Increment(ref _random);
//reactPrefix.Length == 6
chars[6] = _encode32Chars[(int)(id >> 60) & 31];

This comment has been minimized.

@Daniel15

Daniel15 Apr 8, 2018

Member

There's a lot of magic numbers in this code. It'd be good to pull these out as constants and add comments explaining what they're doing.

/// <summary>
/// Extension methods relating to GUIDs.
/// </summary>
public static class ReactIdGenerator

This comment has been minimized.

@Daniel15

Daniel15 Apr 8, 2018

Member

Would be good to add an interface for this, and use the dependency injection framework to inject it into ReactComponent. That way, anyone can swap out the implementation if they want to (eg. if they want the old one back).

This comment has been minimized.

@Daniel15

Daniel15 Apr 8, 2018

Member

(see AssemblyRegistration.cs, use .AsSingleton())

This comment has been minimized.

@DaniilSokolyuk

DaniilSokolyuk Apr 9, 2018

Contributor

Nice idea, i can inject it to ReactEnvironment and create property IReactIdGenerator (like IBabel) and call it from constructor in ReactComponent like "environment.ReactIdGenerator.Generate(...)"

var chars = _chars;
if (chars == null)
{
_chars = chars = new char[19];

This comment has been minimized.

@Daniel15

Daniel15 Apr 8, 2018

Member

Why 19?

chars[17] = _encode32Chars[(int)(id >> 5) & 31];
chars[18] = _encode32Chars[(int)id & 31];
return new string(chars, 0, 19);

This comment has been minimized.

@Daniel15

Daniel15 Apr 8, 2018

Member

Pull out 19 as a constant

This comment has been minimized.

@DaniilSokolyuk

DaniilSokolyuk Apr 9, 2018

Contributor

ok!, const will be inlined without overhead 👍

@Daniel15

This comment has been minimized.

Member

Daniel15 commented Apr 9, 2018

DaniilSokolyuk added some commits Apr 9, 2018

@DaniilSokolyuk

This comment has been minimized.

Contributor

DaniilSokolyuk commented Apr 9, 2018

Completed.
IReactIdGenerator injected to Environment and passed to Component.
GeneratesContainerIdIfNotProvided test deleted because UsesProvidedContainerId is very similar to him

@dustinsoftware

Thanks! Looks good.

@dustinsoftware dustinsoftware merged commit d49bb11 into reactjs:master Apr 12, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
{
public class GuidExtensionsTests
public interface IReactIdGenerator

This comment has been minimized.

@dustinsoftware

dustinsoftware Apr 12, 2018

Collaborator

Ugh, sorry. Missed this one. There is no XML comment for this type which is generating a warning at build time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment