-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix displaying error message with Firefox #167
Conversation
frontend/src/app/config.service.ts
Outdated
retry(3), // retry a failed request up to 3 times | ||
catchError(this.handleError) // then handle the error | ||
); | ||
} | ||
} |
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.
重複して config を取得するリクエストを投げそうだと思いました。
下の様に GET リクエストのobservable を保管しておいて、getConfig が呼ばれたときにまだ config を取得出来ていない場合はそっちを返すのはどうかと思いました。
テストしていませんが、イメージ伝わりやすいかと思ってコード書いてみました。
export class ConfigService {
configUrl = 'assets/config.json';
config: Config;
configObservable: Observable<Config>;
constructor(private http: HttpClient) {}
load() {
if (this.configObservable) return this.configObservable;
this.configObservable = this.loadConfig();
this.configObservable.subscribe(
(config: Config) => {
this.configObservable = null;
this.config = config;
},
error => console.error(error)
);
return this.configObservable;
}
getConfig(): Observable<Config> {
if (this.config) {
return new Observable(observer => {
observer.next(this.config);
observer.complete();
});
} else {
return this.load();
}
}
loadConfig() Observable<Config> {
return this.http.get<Config>(this.configUrl).pipe(
retry(3), // retry a failed request up to 3 times
catchError(this.handleError) // then handle the error
);
}
frontend/src/app/backend.service.ts
Outdated
mergeMap((config: Config) => { | ||
return this.http.get(`${config.backendUrl}/api/block/${query}`); | ||
}) | ||
); |
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.
get(url: string, options: { params?: HttpParams }): Observable<any> {
return this.getConfig().pipe(
mergeMap((config: Config) => {
return this.http.get(url, { params: params });
});
);
みたいなのを作って、
searchBlock(query: string): Observable<any> {
return this.get(`${this.backendUrl}/api/block/${query}`);
);
}
みたいに書けたらすっきりするのではと思いました。何かややこしい事情があってこうしなかったとかあれば無視していただいてOKです。(http.get() のパラメーターがややこしいので、自前の get() のパラメーターの型宣言が面倒でやっていないのかなとか思いました。)
70843ec
to
91ebce2
Compare
91ebce2
to
a109dec
Compare
frontend/src/app/config.service.ts
Outdated
@@ -34,6 +35,17 @@ export class ConfigService { | |||
} | |||
|
|||
getConfig(): Observable<Config> { | |||
if (this.config && this.observable) { |
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.
この条件だと初回のconfig の GET リクエストの応答を待っている間に getConfig()
が呼ばれると、複数回 GET リクエストを投げることになりませんか?
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.
a2c7a99 修正しました。
b83c363
to
a2c7a99
Compare
resolve #166