-
Notifications
You must be signed in to change notification settings - Fork 4.9k
design-system-mcp: Improve concurrency handling for data fetching #78311
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: trunk
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 |
|---|---|---|
|
|
@@ -13,30 +13,58 @@ const DESIGN_TOKENS_URL = | |
| process.env.DESIGN_TOKENS_URL || | ||
| 'https://raw.githubusercontent.com/WordPress/gutenberg/refs/heads/trunk/packages/theme/docs/tokens.md'; | ||
|
|
||
| let cachedComponents: Record< string, ManifestComponent > | null = null; | ||
| let cachedTokens: string | null = null; | ||
| const HOUR_IN_MS = 60 * 60 * 1000; | ||
|
|
||
| /** | ||
| * Clear cached data. Intended for testing. | ||
| */ | ||
| export function resetCache(): void { | ||
| cachedComponents = null; | ||
| cachedTokens = null; | ||
| } | ||
| type CachedFetcher< T > = ( () => Promise< T > ) & { reset: () => void }; | ||
|
|
||
| /** | ||
| * Fetch and cache the components from the Storybook manifest, filtered to only | ||
| * components from allowed packages. | ||
| * Wrap an async fetcher so that successful results are cached for `ttlMs` | ||
| * milliseconds. Concurrent callers share a single in-flight promise, and | ||
| * failed fetches are evicted immediately so the next call can retry. | ||
| * | ||
| * @return The filtered components record. | ||
| * The returned function exposes a `reset()` method that clears its own | ||
| * cache, allowing callers to compose a higher-level reset without the | ||
| * wrapper needing to know about a shared registry. | ||
| * | ||
| * @param fetcher - The async function whose result should be cached. | ||
| * @param ttlMs - Cache lifetime in milliseconds. Defaults to one hour. | ||
| * @return A function returning the cached (or freshly fetched) promise. | ||
| */ | ||
| async function fetchComponents(): Promise< | ||
| Record< string, ManifestComponent > | ||
| > { | ||
| if ( cachedComponents ) { | ||
| return cachedComponents; | ||
| } | ||
| function withTTL< T >( | ||
| fetcher: () => Promise< T >, | ||
| ttlMs: number = HOUR_IN_MS | ||
| ): CachedFetcher< T > { | ||
| let cached: Promise< T > | null = null; | ||
| let expiresAt = 0; | ||
|
|
||
| return Object.assign( | ||
| () => { | ||
| if ( ! cached || Date.now() > expiresAt ) { | ||
| cached = ( async () => { | ||
| try { | ||
| return await fetcher(); | ||
| } catch ( error ) { | ||
| cached = null; | ||
| expiresAt = 0; | ||
| throw error; | ||
| } | ||
| } )(); | ||
|
|
||
| expiresAt = Date.now() + ttlMs; | ||
|
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. Why do we start the timer before the fetch resolves? This comes mostly from a place of curiosity, rather than flagging a bug with the implementation. Maybe worth adding a short code comment about it? |
||
| } | ||
|
|
||
| return cached; | ||
| }, | ||
| { | ||
| reset: () => { | ||
| cached = null; | ||
| expiresAt = 0; | ||
| }, | ||
| } | ||
| ); | ||
| } | ||
|
Comment on lines
+33
to
+65
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. Love this! Although AI-assisted review flagged a potential (albeit rare) edge condition: when the TTL fires while a fetch is still in flght (a 1-hour TTL with a >1-hour-stuck fetch, or Date.now() skewing forward), the next caller falls into the cache-miss branch and overwrites cached with a new IIFE-promise. Both IIFEs are now closed over the same cached and expiresAt variables. If the first IIFE then rejects (after the second has been assigned), its catch handler runs and nukes the second IIFE's cached entry, breaking dedup for anyone arriving between that moment and the second IIFE settling. The same hazard exists with reset() racing an in-flight IIFE, though that one is "by design" (callers asked for a clean cache). Claude suggests this as a potential fixmake the catch identity-check before clearing: () => {
if ( ! cached || Date.now() > expiresAt ) {
const promise: Promise< T > = ( async () => {
try {
return await fetcher();
} catch ( error ) {
if ( cached === promise ) {
cached = null;
expiresAt = 0;
}
throw error;
}
} )();
cached = promise;
expiresAt = Date.now() + ttlMs;
}
return cached;
}an equivalent shape using an outer Again, extremely unlikely but potentially possible, especially if the TTL will every become shorter |
||
|
|
||
| const fetchComponents = withTTL( async () => { | ||
| const response = await fetch( COMPONENTS_MANIFEST_URL ); | ||
| if ( ! response.ok ) { | ||
| throw new Error( | ||
|
|
@@ -56,8 +84,26 @@ async function fetchComponents(): Promise< | |
| } | ||
| } | ||
|
|
||
| cachedComponents = filtered; | ||
| return cachedComponents; | ||
| return filtered; | ||
| } ); | ||
|
|
||
| const fetchTokens = withTTL( async () => { | ||
| const response = await fetch( DESIGN_TOKENS_URL ); | ||
| if ( ! response.ok ) { | ||
| throw new Error( | ||
| `Failed to fetch design tokens: ${ response.status } ${ response.statusText }` | ||
| ); | ||
| } | ||
|
|
||
| return response.text(); | ||
| } ); | ||
|
|
||
| /** | ||
| * Clear all cached data. Intended for testing. | ||
| */ | ||
| export function resetCache(): void { | ||
| fetchComponents.reset(); | ||
| fetchTokens.reset(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -88,17 +134,6 @@ export async function getComponentDetail( | |
| * | ||
| * @return The tokens markdown content. | ||
| */ | ||
| export async function getDesignTokens(): Promise< { content: string } > { | ||
| if ( ! cachedTokens ) { | ||
| const response = await fetch( DESIGN_TOKENS_URL ); | ||
| if ( ! response.ok ) { | ||
| throw new Error( | ||
| `Failed to fetch design tokens: ${ response.status } ${ response.statusText }` | ||
| ); | ||
| } | ||
|
|
||
| cachedTokens = await response.text(); | ||
| } | ||
|
|
||
| return { content: cachedTokens }; | ||
| export function getDesignTokens(): Promise< string > { | ||
| return fetchTokens(); | ||
| } | ||
|
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. Should we add tests targeting the TTL logic? In particular
Another potentially missing test is to check that wwo concurrent callers share the same rejection. Right now the "should evict failed fetches" test fires the failure, awaits its rejection, then makes a second call, but it doesn't check that two concurrent callers awaiting the same in-flight failure both see 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.
Same comment as before re. the readability of an IIFE. Could be potentially rewritten to