-
Notifications
You must be signed in to change notification settings - Fork 408
feat(clerk-js,types): Signals fetchStatus #6549
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
Changes from 4 commits
a2d6ba1
5065c9b
550d029
e2bd52f
5164243
d7f905a
d99ed16
37b024d
bf71255
2336512
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@clerk/clerk-js': minor | ||
| '@clerk/types': minor | ||
| --- | ||
|
|
||
| [Experimental] Signal `fetchStatus` support. |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,20 +4,18 @@ import { computed, signal } from 'alien-signals'; | |||||||
|
|
||||||||
| import type { SignIn } from './resources/SignIn'; | ||||||||
|
|
||||||||
| export const signInSignal = signal<{ resource: SignIn | null }>({ resource: null }); | ||||||||
| export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null }); | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainRename to signInResourceSignal is clear; verify callers and mark as internal Good separation of responsibilities by renaming to signInResourceSignal. If this is not part of the public API, add an @internal JSDoc to comply with docs guidelines and prevent accidental external usage. Also, please verify there are no lingering imports/usages of the old signInSignal. Run this to ensure no leftover references exist: Apply this diff to mark the export as internal: +/** @internal */
export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null });🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find any remaining references to the old name
rg -n --heading '\bsignInSignal\b' -g '!**/node_modules/**'Length of output: 365 Update leftover We’ve got three remaining
Please make these changes to fully remove the old 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||
| export const signInErrorSignal = signal<{ error: unknown }>({ error: null }); | ||||||||
| export const signInFetchSignal = signal<{ status: 'idle' | 'fetching' }>({ status: 'idle' }); | ||||||||
|
|
||||||||
| export const signInComputedSignal = computed(() => { | ||||||||
| const signIn = signInSignal().resource; | ||||||||
| const signIn = signInResourceSignal().resource; | ||||||||
| const error = signInErrorSignal().error; | ||||||||
| const fetchStatus = signInFetchSignal().status; | ||||||||
|
|
||||||||
| const errors = errorsToParsedErrors(error); | ||||||||
|
|
||||||||
| if (!signIn) { | ||||||||
| return { errors, signIn: null }; | ||||||||
| } | ||||||||
|
|
||||||||
| return { errors, signIn: signIn.__internal_future }; | ||||||||
| return { errors, fetchStatus, signIn: signIn ? signIn.__internal_future : null }; | ||||||||
| }); | ||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| /** | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { eventBus } from '../../core/events'; | ||
| import { runAsyncTask } from '../runAsyncTask'; | ||
|
|
||
| describe('runAsyncTask', () => { | ||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| const resource = {} as any; // runAsyncTask doesn't depend on resource being a BaseResource | ||
|
|
||
| it('emits fetching/idle and returns result on success', async () => { | ||
| const emitSpy = vi.spyOn(eventBus, 'emit'); | ||
| const task = vi.fn().mockResolvedValue('ok'); | ||
|
|
||
| const { result, error } = await runAsyncTask(resource, task); | ||
|
|
||
| expect(task).toHaveBeenCalledTimes(1); | ||
| expect(result).toBe('ok'); | ||
| expect(error).toBeNull(); | ||
|
|
||
| expect(emitSpy).toHaveBeenNthCalledWith(1, 'resource:error', { | ||
| resource, | ||
| error: null, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(2, 'resource:fetch', { | ||
| resource, | ||
| status: 'fetching', | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(3, 'resource:fetch', { | ||
| resource, | ||
| status: 'idle', | ||
| }); | ||
| }); | ||
|
|
||
| it('emits error and returns error on failure', async () => { | ||
| const emitSpy = vi.spyOn(eventBus, 'emit'); | ||
| const thrown = new Error('fail'); | ||
| const task = vi.fn().mockRejectedValue(thrown); | ||
|
|
||
| const { result, error } = await runAsyncTask(resource, task); | ||
|
|
||
| expect(task).toHaveBeenCalledTimes(1); | ||
| expect(result).toBeUndefined(); | ||
| expect(error).toBe(thrown); | ||
|
|
||
| expect(emitSpy).toHaveBeenNthCalledWith(1, 'resource:error', { | ||
| resource, | ||
| error: null, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(2, 'resource:fetch', { | ||
| resource, | ||
| status: 'fetching', | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(3, 'resource:error', { | ||
| resource, | ||
| error: thrown, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(4, 'resource:fetch', { | ||
| resource, | ||
| status: 'idle', | ||
| }); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainAdd a concurrency test to catch in-flight status race conditions If two tasks run concurrently for the same resource, the current approach can emit Example test to add: it('handles overlapping tasks without prematurely emitting idle', async () => {
const emitSpy = vi.spyOn(eventBus, 'emit');
let resolveA: (v: unknown) => void;
const taskA = vi.fn(() => new Promise(res => (resolveA = res)));
const taskB = vi.fn().mockResolvedValue('ok');
// Start A (fetching)
const pA = runAsyncTask(resource, taskA);
// Start B (still fetching)
const pB = runAsyncTask(resource, taskB);
// Resolve B first
await pB;
// At this point, we expect no final 'idle' yet if A is still in flight
// Adjust assertion based on desired semantics; e.g., last call isn't 'idle' until A resolves
const lastCall = emitSpy.mock.calls[emitSpy.mock.calls.length - 1];
expect(lastCall[0]).toBe('resource:fetch');
expect(lastCall[1]).toMatchObject({ resource, status: 'fetching' });
// Now resolve A; expect idle
resolveA!('ok');
await pA;
const finalCall = emitSpy.mock.calls[emitSpy.mock.calls.length - 1];
expect(finalCall).toEqual(['resource:fetch', { resource, status: 'idle' }]);
});🏁 Script executed: #!/bin/bash
# Locate the implementation of runAsyncTask to verify concurrency handling
rg -n "export async function runAsyncTask" -C8 packages/clerk-js/src/utilsLength of output: 1346 Enhance The current implementation in To address this, you should: • Introduce an in-flight counter (e.g. a • Add a concurrency test in it('does not emit idle until all overlapping tasks complete', async () => {
const emitSpy = vi.spyOn(eventBus, 'emit');
let resolveA!: (value: unknown) => void;
const taskA = vi.fn(() => new Promise(res => (resolveA = res)));
const taskB = vi.fn().mockResolvedValue('ok');
const pA = runAsyncTask(resource, taskA);
const pB = runAsyncTask(resource, taskB);
await pB;
// Should still be 'fetching' because A is not done
const last = emitSpy.mock.calls.at(-1)!;
expect(last).toEqual(['resource:fetch', { resource, status: 'fetching' }]);
resolveA('ok');
await pA;
// Now finally emit 'idle'
const final = emitSpy.mock.calls.at(-1)!;
expect(final).toEqual(['resource:fetch', { resource, status: 'idle' }]);
});
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dstaley I'm not sure if this will happen in practice, but it's an interesting case to think about. WDYT?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. technically yes, but I don't think there's a valid instance in which it could happen. You would need to trigger two async methods on the |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,30 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { eventBus } from '../core/events'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { BaseResource } from '../core/resources/internal'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Wrap an async task with handling for emitting error and fetch events, which reduces boilerplate. Used in our Custom | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Flow APIs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export async function runAsyncTask<T>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resource: BaseResource, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| task: () => Promise<T>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): Promise<{ result?: T; error: unknown }> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| eventBus.emit('resource:error', { resource, error: null }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| eventBus.emit('resource:fetch', { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resource, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status: 'fetching', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = await task(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { result, error: null }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| eventBus.emit('resource:error', { resource, error: err }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { error: err }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| eventBus.emit('resource:fetch', { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resource, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status: 'idle', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use event constants and guard against overlapping task races with in-flight counting
Apply: -import { eventBus } from '../core/events';
+import { events, eventBus } from '../core/events';
import type { BaseResource } from '../core/resources/internal';
/**
* Wrap an async task with handling for emitting error and fetch events, which reduces boilerplate. Used in our Custom
* Flow APIs.
*/
export async function runAsyncTask<T>(
resource: BaseResource,
task: () => Promise<T>,
): Promise<{ result?: T; error: unknown }> {
- eventBus.emit('resource:error', { resource, error: null });
- eventBus.emit('resource:fetch', {
- resource,
- status: 'fetching',
- });
+ // Keep a per-resource in-flight counter to avoid premature 'idle' on overlapping tasks.
+ // WeakMap ensures resources can be GC'd when no longer referenced.
+ const inflight: WeakMap<BaseResource, number> =
+ // Reuse a shared map on the eventBus instance to avoid module duplication across bundles/tests.
+ // @ts-expect-error: attach internal state
+ (eventBus.__inflight ??= new WeakMap<BaseResource, number>());
+
+ const inc = (inflight.get(resource) ?? 0) + 1;
+ inflight.set(resource, inc);
+
+ eventBus.emit(events.ResourceError, { resource, error: null });
+ if (inc === 1) {
+ eventBus.emit(events.ResourceFetch, {
+ resource,
+ status: 'fetching',
+ });
+ }
try {
const result = await task();
return { result, error: null };
} catch (err) {
- eventBus.emit('resource:error', { resource, error: err });
+ eventBus.emit(events.ResourceError, { resource, error: err });
return { error: err };
} finally {
- eventBus.emit('resource:fetch', {
- resource,
- status: 'idle',
- });
+ const dec = (inflight.get(resource) ?? 1) - 1;
+ if (dec <= 0) {
+ inflight.delete(resource);
+ eventBus.emit(events.ResourceFetch, {
+ resource,
+ status: 'idle',
+ });
+ } else {
+ inflight.set(resource, dec);
+ }
}
}Note: If you’d rather not attach internal state to 📝 Committable suggestion
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dstaley this is an interesting suggestion...do you think this could happen in practice?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same thoughts as #6549 (comment) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Avoid sending identifier: null; include the field only when defined
Using identifier || previousIdentifier can yield null, which will serialize as identifier: null. Prefer nullish coalescing and omit the key when undefined.
📝 Committable suggestion
🤖 Prompt for AI Agents