🎉 feat(scalar): support functions options #281
Conversation
WalkthroughIntroduces a pre-request hook for Scalar via scalar.onBeforeRequest in the example, removes embedSchema usage, refactors Scalar script injection by adding getScriptTags and delegating rendering to it, and relaxes the scalar configuration type to Partial while keeping version/cdn optional. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant ScalarHook as scalar.onBeforeRequest
participant Handler as Route Handler
Client->>Server: HTTP Request
Server->>ScalarHook: Invoke with { request }
Note right of ScalarHook: Logs method and URL
ScalarHook-->>Server: Return (no response change)
Server->>Handler: Proceed to route handling
Handler-->>Client: Response
sequenceDiagram
autonumber
participant ScalarRender
participant getScriptTags
participant Browser
ScalarRender->>getScriptTags: Call with { cdn, ...config, content: embedSpec }
getScriptTags-->>ScalarRender: HTML script tags (config + loader)
ScalarRender-->>Browser: Render <div id="app"> + script tags
Note over Browser: Browser bootstraps API Reference using injected config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/scalar/index.ts (3)
170-178: Simplify indentation logic to improve robustness.The current indentation approach using string splitting, mapping, and regex replacement is fragile. Consider these edge cases:
- Empty
restConfigproduces{without proper structure untilfunctionPropsStringcloses it- Multi-line string values in config may break indentation alignment
- The regex
.replace(/\s*}$/, '')assumes the closing brace is always at the endConsider a simpler approach:
- const configString = JSON.stringify(restConfig, null, 2) - .split('\n') - .map((line, index) => (index === 0 ? line : ' ' + line)) - .join('\n') - .replace(/\s*}$/, '') // Remove the closing brace and any whitespace before it - - const functionPropsString = functionProps.length - ? `,\n ${functionProps.join(',\n ')}\n }` - : '}' + // Build config object parts + const restConfigString = JSON.stringify(restConfig, null, 2) + const configParts = [ + restConfigString.slice(0, -1).trim(), // Remove closing brace + ...functionProps.map(prop => ` ${prop}`) + ] + + const finalConfig = configParts.length > 1 + ? `${configParts[0]},\n${configParts.slice(1).join(',\n')}\n}` + : '{}'
150-156: Improve type safety for configuration entry iteration.The type assertion
as [keyof typeof configuration, unknown][]is necessary becauseObject.entriesloses type information, but this could be made safer.Consider using a type guard or more explicit typing:
for (const key in configuration) { if (!configuration.hasOwnProperty(key)) continue const value = configuration[key as keyof typeof configuration] if (typeof value === 'function') { functionProps.push(`"${key}": ${value.toString()}`) delete restConfig[key as keyof typeof restConfig] } else if (Array.isArray(value) && value.some((item) => typeof item === 'function')) { functionProps.push(`"${key}": ${serializeArrayWithFunctions(value)}`) delete restConfig[key as keyof typeof restConfig] } }
220-221: Avoid mutating the config object.
Object.assign(config, { content: embedSpec })mutates the originalconfigobject, which could cause issues if the config is reused elsewhere.Use object spread to create a new object:
- ${getScriptTags(Object.assign(config, { content: embedSpec }) )} + ${getScriptTags({ ...config, content: embedSpec })}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
example/index.ts(1 hunks)src/scalar/index.ts(2 hunks)src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/scalar/index.ts (1)
src/types.ts (1)
ElysiaOpenAPIConfig(37-215)
🔇 Additional comments (3)
src/types.ts (1)
134-134: LGTM! Type change enables flexible scalar configuration.The change from
ApiReferenceConfigurationtoPartial<ApiReferenceConfiguration>successfully addresses the type error issue mentioned in #280 while preserving the optionalversionandcdnfields. This allows users to provide only the scalar configuration properties they need, including function-valued properties.src/scalar/index.ts (1)
180-188: LGTM! Script generation correctly implements Scalar initialization.The template correctly generates both the CDN script tag and the initialization script. The use of
Scalar.createApiReferencewith the merged configuration (including function properties) aligns with the Scalar API.example/index.ts (1)
13-17: LGTM! Example correctly demonstrates function property support.The addition of
onBeforeRequestsuccessfully demonstrates the new capability to pass function-valued properties in the scalar configuration. With the changes ingetScriptTags, this function will be properly serialized and executed in the browser context, resolving the issue described in #280.
| const serializeArrayWithFunctions = (arr: unknown[]): string => { | ||
| return `[${arr.map((item) => (typeof item === 'function' ? item.toString() : JSON.stringify(item))).join(', ')}]` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify function serialization handles all function types.
The serializeArrayWithFunctions helper uses toString() to serialize functions. Ensure this approach correctly handles:
- Arrow functions
- Async functions
- Generator functions
- Methods with complex closures
Additionally, consider that toString() does not capture the closure scope, so functions referencing external variables may not work as expected when deserialized.
Run the following script to check if there are existing tests for function serialization:
🏁 Script executed:
#!/bin/bash
# Description: Search for tests related to function serialization in scalar configuration
# Search for test files that might cover getScriptTags or serializeArrayWithFunctions
rg -n "getScriptTags|serializeArrayWithFunctions|onBeforeRequest" --type ts --glob '*test*' --glob '*spec*'Length of output: 0
Add unit tests for serializeArrayWithFunctions covering all function forms
serializeArrayWithFunctions relies on toString(), which may not serialize async/generator/arrow functions consistently and doesn’t capture closure scope. Add tests for:
- Arrow functions
- Async functions
- Generator functions
- Functions referencing external variables
There was a problem hiding this comment.
I imagine this is a common pattern where you declare a function on the runtime (Bun) side that then gets injected into the webpage.
So, I wouldn’t add extra checks that could make the code more complex to maintain.
There was a problem hiding this comment.
@marcalexiei, understood! Your point about this being a common pattern for runtime-to-webpage function injection makes sense, and keeping the implementation straightforward is a reasonable trade-off.
🐰
✏️ Learnings added
Learnt from: marcalexiei
PR: elysiajs/elysia-openapi#281
File: src/scalar/index.ts:125-127
Timestamp: 2025-10-09T09:45:40.847Z
Learning: In the elysia-openapi repository, when serializing functions for injection from the runtime (Bun) side into webpages (like in serializeArrayWithFunctions in src/scalar/index.ts), the team prefers to keep the code simple and maintainable rather than adding extensive checks for different function types (arrow, async, generator) or edge cases, as this is a common pattern in their codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
scalaroption property to make all properties fromApiReferenceConfigurationoptionalscalaroption function properties I adapted the code from scalar core packagegetScriptTagsfunctionBasically now instead of using a tag with a stringified json a script is generated with both static and function properties
Summary by CodeRabbit