Skip to content

feat: Keyborg ids should be unique for multiple bundles #75

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

Open
ling1726 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Keyborg ids should be unique for multiple bundles #75

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
21 changes: 20 additions & 1 deletion src/Keyborg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,8 @@ export class Keyborg {
}

private constructor(win: WindowWithKeyborg, props?: KeyborgProps) {
this._id = "k" + ++_lastId;
this._win = win;
this._id = this.createId(win);

const current = win.__keyborg;

Expand All @@ -296,6 +296,25 @@ export class Keyborg {
}
}

/**
* creates an id that will be truly global core instance
* @returns global id for the keyborg instance
*/
private createId(win: WindowWithKeyborg) {
const current = win.__keyborg;
let id = "k" + ++_lastId;
if (!current) {
return id;
}

// other bundles might have created keyborg instances before this one
while (current.refs[id]) {
Copy link
Member

@mshoho mshoho Feb 28, 2024

Choose a reason for hiding this comment

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

This implies that the other __keyborg has refs. And loops through it too.
Let's probably have something like this for createId():

private createId() {
    const rnd = new Uint32Array(4);
    crypto.getRandomValues(rnd);
    return rnd.join('') + '|' + Date.now() + '|' + ++_lastId;
}

It has a pretty good chance to never collide...

Copy link
Contributor Author

@ling1726 ling1726 Feb 28, 2024

Choose a reason for hiding this comment

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

there's only one js thread, right? so they wouldn't loop through the global at the same time?

Copy link
Member

@mshoho mshoho Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's not about thread safety, it's about accessing black box internals assuming it's not a black box.

Copy link

@mathis-m mathis-m May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why not use a different prefix for the new version that is collision save:

k-num

Probably the same should be done for the core id just to get rid of the file scoped thing, and make it version agnostic

id = "k" + ++_lastId;
}

return id;
}

private dispose(): void {
const current = this._win?.__keyborg;

Expand Down