Keep resolved url #118
Keep resolved url #118
Conversation
Borewit
left a comment
Borewit
left a comment
There was a problem hiding this comment.
Please check the url assignment
| private static makeResponse(resp): IRangeRequestResponse { | ||
| const contentRange = HttpClient.parseContentRange(resp.headers); | ||
| return { | ||
| url: resp.url, |
lib/http-client.ts
Outdated
| public getHeadInfo(): Promise<IHeadRequestInfo> { | ||
| return _fetch(this.url, {method: 'HEAD'}).then(resp => HttpClient.makeResponse(resp)); | ||
| return _fetch(this.url, {method: 'HEAD'}).then(resp => { | ||
| this.url = resp.url; |
There was a problem hiding this comment.
Assigning the URL from the response back to the request, looks a bit odd.
There was a problem hiding this comment.
What do you mean by request? Isn't this the instance of the HttpClient? 🤔 The goal here is to change the httpClientInstance.url, or in other words the url used on line 58 as the first argument to _fetch.
There was a problem hiding this comment.
HttpClient maybe is a confusing name. The HttpClient is the request. It is bound to one, and only URL request, one file sessions, which may indeed be multiple HTTP-requests.
There was a problem hiding this comment.
Maybe create a second url attribute for fetching the data?
So fetch does already handle the redirect right? That is why you use the url?
There was a problem hiding this comment.
So fetch does already handle the redirect right? That is why you use the url?
Yes.
This is also a kind of metadata-information, purely used as file identification. Files have a name or URL. The filename may be used to determine the file type. |
|
I think there is no problem following redirects @icidasset. Those redirects may be there for load balancing. Only when a redirect is marked as permanent, you should in principle replace the old URL with new URL. But I see you point, you want to prevent it each range request. I guess the HEAD request is the best place to follow the redirects. |
That's a good point, I didn't think of that 👍 |
|
Ok, so this PR is basically useless except that it now stores the resolved url in the |
|
No it makes sense. I did not directly see why you did it this way, but I think I understand. I would like to keep the originating request URL. If you cache the download URL in another attribute I am happy with it. |
When fetching a url, store the resolved url for later use. This is done, for example, when a user provides a url that redirects. We store the resulting url (after following the redirect), so that it doesn't need to follow the redirect each time we make a request. Thus, this change improves performance when dealing with redirects.
|
Sounds good @Borewit , updated the code. |
|
The HEAD requests may be avoided (because some servers give the wrong response)
|
|
Yeah, I did assign it in |
|
Don't know what the |
| getHeadInfo?(): Promise<IHeadInfo>; | ||
|
|
||
| getResponse(method: string, range?: [number, number]): Promise<IHttpResponse>; | ||
| export interface IHttpClient extends IRangeRequestClient { |
|
Part of v0.5.1 |
|
Thank you @Borewit ! 🙏 🙌 |
|
Thank you @icidasset. |
2 participants
When fetching a url, store the resolved url for later use. This is done, for example, when a user provides a url that redirects. We store the resulting url (after following the redirect), so that it doesn't need to follow the redirect each time we make a request. Thus, this change improves performance when dealing with redirects.
@Borewit This is what I assumed would fix #117
Also related to this, where is
tokenizer.fileInfo.urlused? It appears thisrangeRequestClient(ie. this library) doesn't use that.