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.

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

Use same color for same namespace. #338

Merged
merged 2 commits into from Dec 6, 2016
Merged

Conversation

lchenay
Copy link
Contributor

@lchenay lchenay commented Nov 28, 2016

CF #258

function selectColor(namespace) {
var hash = 0, i, chr, len;

for (i in namespace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this algorithm for somewhere that we need to give credit for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@TooTallNate
Copy link
Contributor

Nice and simple, thanks for the PR! I'm 👍 for this feature. There's been plenty of times where the order of require calls changing ends up changing the colors of the debug instances, since they get instantiated in a different order which has always felt strange.

The only thing I'd be worried about is if the algorithm doesn't "distribute" the colors evenly enough. But I guess we'll just have to feel it out and see.

@lchenay
Copy link
Contributor Author

lchenay commented Nov 28, 2016

I agree, it will have less perfect distribution on colors over namespace.
To reduce chance of collision, as it's no more incremental, the only solution is to have more colors available.

@thebigredgeek thebigredgeek merged commit 501521f into debug-js:master Dec 6, 2016
@Meithar
Copy link

Meithar commented Dec 22, 2016

This algorithm doesn't seem to work well. Example:

console.log({
  namespace,
  hash: Math.abs(hash),
  color: exports.colors[Math.abs(hash) % exports.colors.length],
});
{namespace: "rto-ext:EVENTS", hash: 1808327694, color: "lightseagreen"}
{namespace: "rto-ext:SETTINGS", hash: 1479630660, color: "lightseagreen"}
{namespace: "rto-ext:BG", hash: 33268638, color: "lightseagreen"}

Three different namespaces have the same color.

@lchenay lchenay deleted the patch-1 branch December 22, 2016 12:47
@lchenay
Copy link
Contributor Author

lchenay commented Dec 22, 2016

exports.colors.length == 6
So 16% of chance of collision.

@TooTallNate More color ?

Otherwise we need to have an array to store the colors stickiness. Not as complicated as this.

@lchenay
Copy link
Contributor Author

lchenay commented Dec 22, 2016

We can try something like that otherwise : lchenay@9f4404f

The color will be better distributed as before, with reuse of same color for same namespace, but will use some memory to store the array of mapping.
This can lead to memory leak if a user use debug in prod with random namespace. For exemple those who use a namespace like "myapp:request:${request.expressRequestId}".

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

Successfully merging this pull request may close these issues.

None yet

4 participants