refactor: simplify token options API by flattening scope parameter - #4
Conversation
- Flatten token options structure from { authorizationParams: { scope } } to { scope }
- Remove TokenSupplierWithScopes and SimpleTokenSupplier types in favor of single TokenSupplier
- Remove isTokenSupplierWithScopes type guard
- Simplify createCoreTokenSupplier to always call token supplier with scope when available
- Update all examples and documentation to use new flattened structure
- Add comprehensive Token.test.ts test suite
- Update .gitignore and .fernignore for better file management
This change makes the API more intuitive by allowing users to destructure
{ scope } directly instead of { authorizationParams: { scope } }. The SDK
always calls the function with a scope object when scopes are present, or
no argument when there are no scopes. JavaScript's flexibility allows
functions to ignore extra parameters, supporting both patterns seamlessly.
Following the same refactoring pattern as myaccount-js PR #4.
Update example functions to properly handle optional parameter: - Use options?.scope with fallback to empty string - Update getAccessToken function signature to accept optional parameter - Fixes TypeScript errors from destructuring potentially undefined value
|
Claude finished @developerkunal's task —— View job Overall Assessment❌ The PR contains a critical logic flaw in |
| return async ({ endpointMetadata }) => { | ||
| if (isTokenSupplierWithScopes(tokenSupplier)) { | ||
| const scopes = extractScopesFromMetadata(endpointMetadata); | ||
| const scope = scopes.join(" "); | ||
| return await tokenSupplier({ | ||
| authorizationParams: { scope }, | ||
| }); | ||
| const scopes = extractScopesFromMetadata(endpointMetadata); | ||
| const scope = scopes.join(" "); | ||
|
|
||
| if (scope) { | ||
| return await tokenSupplier({ scope }); | ||
| } | ||
| return await (tokenSupplier as Auth0Token.SimpleTokenSupplier)(); | ||
| return await tokenSupplier(); |
There was a problem hiding this comment.
Critical Issue: Logic flaw in scope handling
The current implementation only calls tokenSupplier({ scope }) when scope is non-empty, but calls tokenSupplier() when scope is empty. This is problematic because:
-
Type inconsistency: The function signature is
(options?: TokenOptions), meaning it expects either an object parameter or no parameter. However, when called without arguments, functions that destructure{ scope }will receiveundefinedfor their parameter, not an empty object. -
Breaking change for existing code: Functions written as
({ scope }) => ...will fail when called with no arguments, since destructuring undefined throws an error. -
Inconsistent behavior: Token suppliers that check for the presence of the parameter will behave differently based on whether scopes are present or not.
Recommended fix:
if (typeof tokenSupplier === "function") {
return async ({ endpointMetadata }) => {
const scopes = extractScopesFromMetadata(endpointMetadata);
const scope = scopes.join(" ");
// Always call with options object for consistency
return await tokenSupplier({ scope });
};
}This ensures:
- Consistent behavior regardless of scope presence
- Functions can safely destructure
{ scope } - The
scopewill be an empty string when no scopes are required, which is semantically correct
|
Claude finished @developerkunal's task —— View job Overall Assessment✅ This PR successfully simplifies the token options API and is ready to merge after addressing minor documentation clarity issues. The core implementation is sound, well-tested, and the critical issue from the previous review has been correctly resolved. Strengths:
Minor Issues (Documentation Clarity):
Code Quality: ✅ Excellent
Security: ✅ No concerns identified
Performance: ✅ No concerns identified
Breaking Change Management: ✅ Well documented
|
- Clarify that JS ignores extra arguments for functions without parameters
- Make it explicit that SDK always passes { scope }, not conditional behavior
- Add comment explaining empty scope split edge case in deduplication test
- Add dedicated test for empty scope string edge case (split behavior)
- Address bot review feedback for better documentation precision
Changes
This PR simplifies the token options API by flattening the scope parameter structure, following the same pattern as myaccount-js PR #4.
API Simplification:
token: async ({ authorizationParams }) => { scope: authorizationParams.scope }token: async ({ scope }) => { scope }Classes and methods changed:
Auth0Token.TokenOptionsinterface - Flattened from nested structure to singlescope: stringpropertyAuth0Token.TokenSupplierWithScopesandAuth0Token.SimpleTokenSuppliertypesAuth0Token.TokenSuppliertype with optional parametercreateCoreTokenSupplierfunction - Simplified implementation (removed type guard logic)isTokenSupplierWithScopestype guard functionFiles modified:
example.ts- Updated 3 examples with flattened scope structuresrc/wrappers/MyOrganizationClient.ts- Updated JSDoc examples (6 occurrences)src/wrappers/auth/Token.ts- Major refactoring of type system and implementationtests/unit/wrappers/MyOrganizationClient.test.ts- Updated test expectations (2 occurrences)tests/unit/wrappers/auth/Token.test.ts- NEW: Comprehensive test suite for Token module.gitignore- Addeddocs/to exclusions.fernignore- Added.gitignoreUsage example (new API):
Benefits:
{ scope }directlyTokenSuppliertype instead of multiple variantsBreaking Change Migration:
References
Testing
All changes are fully tested and validated:
Test coverage:
Token.test.tswith 16 test cases covering:Manual testing:
Checklist