-
Notifications
You must be signed in to change notification settings - Fork 346
feat(clerk-js): Track fapi requests triggered by UI components #5299
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
base: main
Are you sure you want to change the base?
Changes from all commits
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,5 @@ | ||
--- | ||
'@clerk/clerk-js': minor | ||
--- | ||
|
||
Track fapi requests triggered by UI components. |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,49 @@ | ||||||||
function createStore(initialUsages: number) { | ||||||||
let state = initialUsages; | ||||||||
|
||||||||
return { | ||||||||
get: () => state > 0, | ||||||||
increment: () => { | ||||||||
state++; | ||||||||
}, | ||||||||
decrease: () => { | ||||||||
state = Math.max(state - 1, 0); | ||||||||
}, | ||||||||
}; | ||||||||
} | ||||||||
|
||||||||
export const usageByUIComponents = createStore(0); | ||||||||
|
||||||||
const isThenable = (value: unknown): value is Promise<unknown> => { | ||||||||
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.
Suggested change
🙃 I'd suggest updating the name to match the type predicate as well, |
||||||||
return !!value && typeof (value as any).then === 'function'; | ||||||||
}; | ||||||||
|
||||||||
export const makeUICaller = <T>(resource: T): T => { | ||||||||
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.
Suggested change
🙃 We could add JSDocs to indicate for external contributors that this is a tooling for internal usage |
||||||||
if (!resource) return null as T; | ||||||||
// return resource | ||||||||
const resourceProxy = new Proxy(resource, { | ||||||||
get(target, prop) { | ||||||||
// @ts-expect-error | ||||||||
const value = target[prop]; | ||||||||
if (typeof value === 'function') { | ||||||||
// @ts-expect-error | ||||||||
return function (...args) { | ||||||||
usageByUIComponents.increment(); | ||||||||
const result = value.apply(target, args); | ||||||||
if (isThenable(result)) { | ||||||||
return result.finally(() => usageByUIComponents.decrease()); | ||||||||
} | ||||||||
usageByUIComponents.decrease(); | ||||||||
return result; | ||||||||
}; | ||||||||
} | ||||||||
|
||||||||
// Allows for nested objects to be proxied (e.g. `client.signIn.create()`) | ||||||||
if (typeof value === 'object') { | ||||||||
return makeUICaller(value); | ||||||||
} | ||||||||
return value; | ||||||||
}, | ||||||||
}); | ||||||||
return resourceProxy; | ||||||||
}; |
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 against this approach, mostly curious regarding other options - have you thought of forwarding a property to
Clerk
if it loads on a React context, and then accessing this on the FAPI client?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.
Can you share more about this.
Clerk
is already loaded when we mount the providers and the UI components.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.
Once the
CoreClerkContextWrapper
loads, it sets a property inClerk
that can be accessed on thefapiClient
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 think this increases the margin of faulty detections by a lot, since all requests triggered outside of a UI component will be flagged incorrectly while a UI component is mounted.