-
-
Notifications
You must be signed in to change notification settings - Fork 89
Migrate kg-unsplash-selector to TypeScript #1762
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: main
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 |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ export interface IUnsplashService { | |
| updateSearch(term: string): Promise<void>; | ||
| loadNextPage(): Promise<void>; | ||
| clearPhotos(): void; | ||
| triggerDownload(photo: Photo): void; | ||
| triggerDownload(photo: Pick<Photo, 'links'>): void; | ||
|
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. Interface declares The Consider making this method async and returning the promise: 🔧 Proposed fix export interface IUnsplashService {
// ...
- triggerDownload(photo: Pick<Photo, 'links'>): void;
+ triggerDownload(photo: Pick<Photo, 'links'>): Promise<void>;
// ...
}- triggerDownload(photo: Pick<Photo, 'links'>) {
- this.photoUseCases.triggerDownload(photo);
+ async triggerDownload(photo: Pick<Photo, 'links'>): Promise<void> {
+ await this.photoUseCases.triggerDownload(photo);
}Also applies to: 64-66 🤖 Prompt for AI Agents |
||
| photos: Photo[]; | ||
| searchIsRunning(): boolean; | ||
| } | ||
|
|
@@ -61,7 +61,7 @@ export class UnsplashService implements IUnsplashService { | |
| this.photos = []; | ||
| } | ||
|
|
||
| triggerDownload(photo: Photo) { | ||
| triggerDownload(photo: Pick<Photo, 'links'>) { | ||
| this.photoUseCases.triggerDownload(photo); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,29 +1,21 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "compilerOptions": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "target": "ESNext", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "useDefineForClassFields": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "module": "ESNext", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "moduleResolution": "bundler", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "lib": ["DOM", "DOM.Iterable", "ESNext"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "allowJs": false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "allowSyntheticDefaultImports": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "jsx": "react-jsx", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "strict": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "declaration": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "declarationMap": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sourceMap": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "esModuleInterop": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "forceConsistentCasingInFileNames": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "module": "ESNext", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "moduleResolution": "Node", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "skipLibCheck": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "resolveJsonModule": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "isolatedModules": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "jsx": "react-jsx", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "skipLibCheck": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "esModuleInterop": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "declarationMap": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "declaration": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "emitDeclarationOnly": true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "declarationDir": "./types", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "baseUrl": ".", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "paths": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "kg-unsplash-selector": ["src/index.ts"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "typeRoots": ["node_modules/@types", "types/index.d.ts"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "noEmit": true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+9
to
+17
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify whether this package actually needs emitted declaration files.
# Expected:
# - If package publishes types from build output, keep declaration flags in a build-specific tsconfig.
# - If package is type-check only, declaration flags can be removed from this file.
set -euo pipefail
echo "== tsconfig compilerOptions =="
jq '.compilerOptions | {noEmit, declaration, declarationMap, sourceMap}' packages/kg-unsplash-selector/tsconfig.json
echo
echo "== package metadata that may require emitted types =="
jq '{name, types, typings, files, exports}' packages/kg-unsplash-selector/package.jsonRepository: TryGhost/Koenig Length of output: 412 🏁 Script executed: #!/bin/bash
# Search for all tsconfig files in the package
echo "=== tsconfig files in kg-unsplash-selector package ==="
find packages/kg-unsplash-selector -name "tsconfig*.json" -type f
echo
echo "=== package.json build scripts ==="
jq '.scripts' packages/kg-unsplash-selector/package.json
echo
echo "=== Check if dist directory exists ==="
ls -la packages/kg-unsplash-selector/ | grep -E "^d|^-"Repository: TryGhost/Koenig Length of output: 2035 🏁 Script executed: #!/bin/bash
echo "=== tsconfig.declaration.json ==="
cat packages/kg-unsplash-selector/tsconfig.declaration.json
echo
echo "=== tsconfig.node.json ==="
cat packages/kg-unsplash-selector/tsconfig.node.json
echo
echo "=== Main tsconfig.json (full file) ==="
cat packages/kg-unsplash-selector/tsconfig.jsonRepository: TryGhost/Koenig Length of output: 1126 🏁 Script executed: #!/bin/bash
echo "=== vite.config.ts (check output paths) ==="
cat packages/kg-unsplash-selector/vite.config.ts
echo
echo "=== Check if 'types' output handling in package.json ==="
head -20 packages/kg-unsplash-selector/package.jsonRepository: TryGhost/Koenig Length of output: 1716 Remove inert emit-related flags from tsconfig.json. The flags You can safely delete lines 9–11 to reduce configuration noise and prevent future confusion. Cleanup "strict": true,
- "declaration": true,
- "declarationMap": true,
- "sourceMap": true,
"esModuleInterop": true,📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "include": ["src"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "references": [{ "path": "./tsconfig.node.json" }], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "include": ["src/**/*"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "references": [{ "path": "./tsconfig.node.json" }] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| { | ||
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "rootDir": ".", | ||
| "outDir": null, | ||
| "noEmit": true, | ||
| "declaration": false, | ||
| "declarationMap": false, | ||
| "sourceMap": false, | ||
| "incremental": false | ||
| }, | ||
| "include": [ | ||
| "src/**/*", | ||
| "test/**/*" | ||
| ] | ||
| } |
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.
🧩 Analysis chain
🏁 Script executed:
Repository: TryGhost/Koenig
Length of output: 3273
triggerDownloaddrops provider failures due to missing await.On line 20,
this._provider.triggerDownload(photo)is invoked inside anasyncmethod but not awaited or returned. If the provider returns a rejecting promise, it will escape unhandled. Since the provider interface declares the return type asPromise<void> | void, useawaitto handle both synchronous and asynchronous cases.Additionally,
packages/kg-unsplash-selector/src/api/UnsplashService.tsneeds attention: itstriggerDownloadmethod (lines 64-66) should either propagate the promise by returning it, or change the interface signature fromvoidtoPromise<void>to reflect the actual behavior after fixing PhotoUseCase.✅ Proposed fix (PhotoUseCase.ts)
📝 Committable suggestion
🤖 Prompt for AI Agents