Add Node.js Quickstart - #32
Conversation
📝 WalkthroughWalkthroughThe JavaScript SDK adds OAuth 2.0 ChangesClient credentials flow
Express quickstart configuration update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Quickstart
participant AuthService
participant ThunderIDNodeClient
participant TokenEndpoint
participant InventoryService
Quickstart->>AuthService: initialize client credentials
AuthService->>ThunderIDNodeClient: request cached access token
ThunderIDNodeClient->>TokenEndpoint: request client_credentials token when cache is stale
TokenEndpoint-->>ThunderIDNodeClient: return access token
ThunderIDNodeClient-->>AuthService: return access token
Quickstart->>InventoryService: request SKU stock
InventoryService->>AuthService: obtain access token
InventoryService-->>Quickstart: return inventory result or error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ 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 |
brionmario
changed the title
b59aa83 to
543f85d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
samples/node/quickstart/.env.example (1)
1-8: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winComment out
NODE_TLS_REJECT_UNAUTHORIZED=0by default.The variable is active by default when a developer copies this file to
.env, even though the preceding comment warns against it. Make it opt-in by commenting it out, so a developer must consciously uncomment it after reading the warning.🛡️ Proposed fix to make the TLS bypass opt-in
# DANGER: Disables ALL TLS verification. Only for local development with self-signed certs. NEVER use in production. -NODE_TLS_REJECT_UNAUTHORIZED=0 +#NODE_TLS_REJECT_UNAUTHORIZED=0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@samples/node/quickstart/.env.example` around lines 1 - 8, Comment out the NODE_TLS_REJECT_UNAUTHORIZED=0 entry in the environment template while preserving its warning, making TLS verification bypass opt-in when developers copy the file to .env.
🧹 Nitpick comments (2)
packages/javascript/src/ThunderIDJavaScriptClient.ts (1)
797-897: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFix the
any-typed catch and align the fetch config with sibling methods.Static analysis flags line 865 (
catch (error: any),@typescript-eslint/no-explicit-any) and line 869 (unsafeanyargument passed where astringis expected,@typescript-eslint/no-unsafe-argument). Type the caught error and coerce it before use, as the sibling exception messages expect astring.Separately, the
fetchcall at lines 860-864 omitscredentials, unlikerequestAccessToken,exchangeToken, andrefreshAccessToken, which all setcredentials: configData.sendCookiesInRequests ? 'include' : 'same-origin'. If this omission is intentional for machine-to-machine requests, consider a short comment explaining the divergence; otherwise, align it with the other token-request methods for consistency.🔧 Proposed fix for the `any`-typed catch block
- } catch (error: any) { + } catch (error: unknown) { throw new ThunderIDAuthException( 'JS-AUTH_CORE-RCCT-NE02', 'Requesting client credentials token failed.', - error ?? 'The request to get the client credentials access token failed.', + error instanceof Error ? error.message : 'The request to get the client credentials access token failed.', ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/ThunderIDJavaScriptClient.ts` around lines 797 - 897, Update requestClientCredentialsToken to use an unknown-typed catch variable and convert the caught value to a string before passing it to ThunderIDAuthException. Also align its fetch configuration with requestAccessToken, exchangeToken, and refreshAccessToken by setting credentials from configData.sendCookiesInRequests, or document the intentional machine-to-machine divergence with a concise comment.Source: Linters/SAST tools
samples/node/quickstart/package.json (1)
7-9: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRequire Node.js 22.9+ for the quickstart scripts.
npm startandnpm run devuse--env-file-if-exists, butsamples/node/quickstart/README.mddocuments Node.js 18+. Either require Node.js 22.9+ insamples/node/quickstart/README.mdandpackage.json, or replace the flag with a Node 18-compatible mechanism for optional env loading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@samples/node/quickstart/package.json` around lines 7 - 9, Align the quickstart’s documented and declared Node.js requirement with the --env-file-if-exists usage in the start and dev scripts: update the README.md prerequisite and package.json engines metadata to require Node.js 22.9 or newer. Keep both scripts unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/javascript/src/ThunderIDJavaScriptClient.ts`:
- Around line 186-214: Add an in-flight token-request promise around
getAccessToken’s client_credentials path so concurrent callers share one
requestClientCredentialsToken invocation when the cached token is missing or
stale. Reuse the shared promise’s result for all callers and clear it after
completion, including rejection, so later calls can retry; preserve existing
cached-token and non-client_credentials behavior.
In `@samples/express/quickstart/package.json`:
- Around line 7-8: Update the quickstart runtime compatibility by either
replacing --env-file-if-exists in the start and dev scripts with an
environment-file approach supported by the documented Node.js 18+ range, or
raise the README’s required Node.js version to 22.9+ so it matches both scripts.
---
Outside diff comments:
In `@samples/node/quickstart/.env.example`:
- Around line 1-8: Comment out the NODE_TLS_REJECT_UNAUTHORIZED=0 entry in the
environment template while preserving its warning, making TLS verification
bypass opt-in when developers copy the file to .env.
---
Nitpick comments:
In `@packages/javascript/src/ThunderIDJavaScriptClient.ts`:
- Around line 797-897: Update requestClientCredentialsToken to use an
unknown-typed catch variable and convert the caught value to a string before
passing it to ThunderIDAuthException. Also align its fetch configuration with
requestAccessToken, exchangeToken, and refreshAccessToken by setting credentials
from configData.sendCookiesInRequests, or document the intentional
machine-to-machine divergence with a concise comment.
In `@samples/node/quickstart/package.json`:
- Around line 7-9: Align the quickstart’s documented and declared Node.js
requirement with the --env-file-if-exists usage in the start and dev scripts:
update the README.md prerequisite and package.json engines metadata to require
Node.js 22.9 or newer. Keep both scripts unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d15428fd-ec74-42f6-a234-867d812935c9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
packages/javascript/src/ThunderIDJavaScriptClient.tspackages/javascript/src/constants/TokenConstants.tspackages/javascript/src/models/config.tssamples/express/quickstart/index.mjssamples/express/quickstart/package.jsonsamples/express/quickstart/public/styles.csssamples/node/quickstart/.env.examplesamples/node/quickstart/README.mdsamples/node/quickstart/index.mjssamples/node/quickstart/lib/AuthService.mjssamples/node/quickstart/lib/InventoryService.mjssamples/node/quickstart/lib/ui.mjssamples/node/quickstart/package.json
| public async getAccessToken(sessionId?: string): Promise<string> { | ||
| const configData = await this.configProvider(); | ||
|
|
||
| if (configData.grantType === 'client_credentials') { | ||
| const sessionData = await this.storageManager.getSessionData(sessionId); | ||
|
|
||
| if (sessionData?.access_token && this.isCachedTokenStillValid(sessionData)) { | ||
| return sessionData.access_token; | ||
| } | ||
|
|
||
| const tokenResponse = await this.requestClientCredentialsToken(sessionId); | ||
|
|
||
| return tokenResponse.accessToken; | ||
| } | ||
|
|
||
| return (await this.storageManager.getSessionData(sessionId))?.access_token; | ||
| } | ||
|
|
||
| private isCachedTokenStillValid(sessionData: SessionData): boolean { | ||
| if (!sessionData.created_at || !sessionData.expires_in) { | ||
| return false; | ||
| } | ||
|
|
||
| const expiresInMs = parseInt(sessionData.expires_in, 10) * 1000; | ||
|
|
||
| return ( | ||
| sessionData.created_at + expiresInMs - TokenConstants.Lifecycle.CLIENT_CREDENTIALS_REFRESH_MARGIN_MS > Date.now() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add request coalescing for concurrent client_credentials token fetches.
When the cached token is missing or stale, every concurrent caller of getAccessToken() independently calls requestClientCredentialsToken. Under concurrent load (the typical case for a machine-to-machine client backing a busy service), this sends multiple simultaneous requests to the token endpoint for the same client. This can trigger rate limiting on the authorization server or add avoidable load during a stampede.
Cache the in-flight request promise so concurrent callers await the same request instead of issuing separate ones.
🔧 Proposed fix to coalesce concurrent client-credentials requests
+ private clientCredentialsTokenPromise?: Promise<TokenResponse>;
+
public async getAccessToken(sessionId?: string): Promise<string> {
const configData = await this.configProvider();
if (configData.grantType === 'client_credentials') {
const sessionData = await this.storageManager.getSessionData(sessionId);
if (sessionData?.access_token && this.isCachedTokenStillValid(sessionData)) {
return sessionData.access_token;
}
- const tokenResponse = await this.requestClientCredentialsToken(sessionId);
+ if (!this.clientCredentialsTokenPromise) {
+ this.clientCredentialsTokenPromise = this.requestClientCredentialsToken(sessionId).finally(() => {
+ this.clientCredentialsTokenPromise = undefined;
+ });
+ }
+
+ const tokenResponse = await this.clientCredentialsTokenPromise;
return tokenResponse.accessToken;
}
return (await this.storageManager.getSessionData(sessionId))?.access_token;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async getAccessToken(sessionId?: string): Promise<string> { | |
| const configData = await this.configProvider(); | |
| if (configData.grantType === 'client_credentials') { | |
| const sessionData = await this.storageManager.getSessionData(sessionId); | |
| if (sessionData?.access_token && this.isCachedTokenStillValid(sessionData)) { | |
| return sessionData.access_token; | |
| } | |
| const tokenResponse = await this.requestClientCredentialsToken(sessionId); | |
| return tokenResponse.accessToken; | |
| } | |
| return (await this.storageManager.getSessionData(sessionId))?.access_token; | |
| } | |
| private isCachedTokenStillValid(sessionData: SessionData): boolean { | |
| if (!sessionData.created_at || !sessionData.expires_in) { | |
| return false; | |
| } | |
| const expiresInMs = parseInt(sessionData.expires_in, 10) * 1000; | |
| return ( | |
| sessionData.created_at + expiresInMs - TokenConstants.Lifecycle.CLIENT_CREDENTIALS_REFRESH_MARGIN_MS > Date.now() | |
| ); | |
| } | |
| private clientCredentialsTokenPromise?: Promise<TokenResponse>; | |
| public async getAccessToken(sessionId?: string): Promise<string> { | |
| const configData = await this.configProvider(); | |
| if (configData.grantType === 'client_credentials') { | |
| const sessionData = await this.storageManager.getSessionData(sessionId); | |
| if (sessionData?.access_token && this.isCachedTokenStillValid(sessionData)) { | |
| return sessionData.access_token; | |
| } | |
| if (!this.clientCredentialsTokenPromise) { | |
| this.clientCredentialsTokenPromise = this.requestClientCredentialsToken(sessionId).finally(() => { | |
| this.clientCredentialsTokenPromise = undefined; | |
| }); | |
| } | |
| const tokenResponse = await this.clientCredentialsTokenPromise; | |
| return tokenResponse.accessToken; | |
| } | |
| return (await this.storageManager.getSessionData(sessionId))?.access_token; | |
| } | |
| private isCachedTokenStillValid(sessionData: SessionData): boolean { | |
| if (!sessionData.created_at || !sessionData.expires_in) { | |
| return false; | |
| } | |
| const expiresInMs = parseInt(sessionData.expires_in, 10) * 1000; | |
| return ( | |
| sessionData.created_at + expiresInMs - TokenConstants.Lifecycle.CLIENT_CREDENTIALS_REFRESH_MARGIN_MS > Date.now() | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/ThunderIDJavaScriptClient.ts` around lines 186 - 214,
Add an in-flight token-request promise around getAccessToken’s
client_credentials path so concurrent callers share one
requestClientCredentialsToken invocation when the cached token is missing or
stale. Reuse the shared promise’s result for all callers and clear it after
completion, including rejection, so later calls can retry; preserve existing
cached-token and non-client_credentials behavior.
| "start": "node --env-file-if-exists=.env index.mjs", | ||
| "dev": "node --env-file-if-exists=.env --watch index.mjs" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob 'package.json' --glob '*.md' \
'"engines"|Node\.js|nodejs|--env-file-if-exists|22\.9' .Repository: thunder-id/javascript-sdks
Length of output: 1997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Express quickstart package:\n'
cat -n samples/express/quickstart/package.json
printf '\nExpress quickstart README runtime line:\n'
sed -n '1,45p' samples/express/quickstart/README.md | cat -n
printf '\nAll express quickstart node scripts in package.json:\n'
python3 - <<'PY'
import json
from pathlib import Path
p=Path('samples/express/quickstart/package.json')
data=json.loads(p.read_text())
for k,v in data.get('scripts',{}).items():
print(k, v)
PYRepository: thunder-id/javascript-sdks
Length of output: 3358
🌐 Web query:
Node.js CLI change log --env-file-if-exists added version
💡 Result:
The Node.js CLI flag --env-file-if-exists was added in version v22.9.0 [1][2][3]. This flag functions similarly to --env-file by loading environment variables from a specified file into process.env, but it differs in that it does not throw an error if the specified file does not exist [1][4]. The --env-file-if-exists flag was originally introduced as an experimental feature and was later marked as stable in Node.js versions v24.10.0 and v22.21.0 [1][4][5].
Citations:
- 1: https://nodejs.org/dist/latest/docs/api/cli.html
- 2: https://nodejs.org/docs/v22.11.0/api/cli.html
- 3: https://env.dev/guides/nodejs-env-variables
- 4: https://nodejs.org/docs/latest-v24.x/api/cli.html
- 5: https://nodejs.org/api/cli.html
Require Node.js 22.9+ or make the env-file loader compatible.
The README documents Node.js 18+, but --env-file-if-exists is only available from Node.js v22.9.0. Users on 18–21 will get an invalid option before index.mjs starts. Raise the supported runtime to 22.9+ or use a compatible optional-environment-file loader.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@samples/express/quickstart/package.json` around lines 7 - 8, Update the
quickstart runtime compatibility by either replacing --env-file-if-exists in the
start and dev scripts with an environment-file approach supported by the
documented Node.js 18+ range, or raise the README’s required Node.js version to
22.9+ so it matches both scripts.
2 participants
Purpose
Approach
N/A
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
client_credentialsauthentication support with automatic access-token caching and refresh.Documentation
Style