-
Notifications
You must be signed in to change notification settings - Fork 334
chore(clerk-js,shared): Improve createEventBus
and move it to shared
#5546
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
Conversation
🦋 Changeset detectedLatest commit: c9167b8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
'@clerk/shared': minor | ||
--- | ||
|
||
Export `createEventBus` from `@clerk/shared/eventBus`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Why does this need to be public now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Clerk.status, isomorphic Clerk also uses an event bus until clerk-js hydrates.
createEventBus
and move it to shared
Co-authored-by: Jacek Radko <[email protected]>
bus.on('user:login', onLogin); | ||
|
||
// Subscribe with priority (runs before regular handlers) | ||
bus.onBefore('error', (error) => console.error('Error occurred:', error)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of the naming here, I read it as "on 'before' event". How about preOn
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of the current name either. How about prioritizedOn/Off
?
# Conflicts: # packages/shared/package.json
on: (...args) => _on(eventToHandlersMap, latestPayloadMap, ...args), | ||
// Subscribe to an event with priority | ||
// Registered handlers with `prioritizedOn` will be called before handlers registered with `on` | ||
prioritizedOn: (...args) => _on(eventToPredispatchHandlersMap, latestPayloadMap, ...args), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be an option that is set on the regular on
and off
methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, and I tried that, it would make it go over 400bytes, and i decided to revert it. Do you feel strongly for about it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacekradko Keep in mind this will still be internal, it's just exported by shared.
Developers doing clerk.on()
will not be able to do clerk. prioritizedOn()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@panteliselef I don't think it's too problematic, just a a larger API surface. We can always revisit in the future
Description
Extract createEventBus changes from #5476.
New APIs
onBefore()
is triggered after.emit()
but its listeners are executed before the listeners ofon()
.offBefore()
unregistersonBefore
internal.retrieveListeners()
useful of lopping through listeners and attaching them to a new event bus instance. Exactly what we need when "hydrating" in isomorphic clerkReplaced APIs
Example
Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change