-
Notifications
You must be signed in to change notification settings - Fork 22
fix: type fix for the build issue #38
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
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested reviewers
Poem
β¨ Finishing Touches
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. πͺ§ TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
π§Ή Nitpick comments (1)
src/app/services/captcha-service/captcha.service.ts (1)
8-19: Consider caching the in-flight promise to avoid duplicate script injections
Repeated calls toloadScript()while the first request is still pending will append multiple<script>elements to the DOM. A lightweight improvement is to cache the promise once created and reuse it for subsequent callers.export class CaptchaService { private scriptLoaded = false; + private scriptLoadingPromise: Promise<void> | null = null; loadScript(): Promise<void> { - return new Promise<void>((resolve, reject) => { + if (this.scriptLoaded) { + return Promise.resolve(); + } + if (this.scriptLoadingPromise) { + return this.scriptLoadingPromise; + } + + this.scriptLoadingPromise = new Promise<void>((resolve, reject) => { if (this.scriptLoaded) return resolve(); @@ }; document.head.appendChild(script); }); + + return this.scriptLoadingPromise; } }This keeps the public API unchanged while preventing duplicate network requests and extraneous DOM nodes.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
src/app/services/captcha-service/captcha.service.ts(1 hunks)
π§° Additional context used
𧬠Code Graph Analysis (1)
src/app/services/captcha-service/captcha.service.ts (1)
src/environments/environment.local.ts (1)
environment(42-56)
β° Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: package
- GitHub Check: Analyze (javascript)
π Additional comments (1)
src/app/services/captcha-service/captcha.service.ts (1)
8-11: Type-safety fix is correct and idiomatic
Changing the return type toPromise<void>and dropping the redundantundefinedpayload aligns with the TypeScript 2.4 constraint and cleans up the call-sites.
π Description
JIRA ID: AMM-1247
Captcha service script loading method return type compatibility with typescript 2.4 has been fixed in this commit
β Type of Change
Summary by CodeRabbit