-
Notifications
You must be signed in to change notification settings - Fork 9
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
Follow redirects in the loader #1099
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 |
---|---|---|
|
@@ -495,17 +495,53 @@ export class Loader { | |
} | ||
} | ||
|
||
// For following redirects of responses returned by loader's urlHandlers | ||
private async simulateFetch( | ||
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. I guess this is good in that it removes the assumptions of the mocked response. Tho the mocked response was contained in tests. Does this code path actually get passed other than tests? 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. I think there is currently no url handler in the non-test code that would result in a redirect. Tests use 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.
Not in tests, I think most redirects occur when clicking the pills. The url fetched is 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. Hm, I would think that in that case the native fetch is used which follows redirections automatically (by default) |
||
request: Request, | ||
result: Response, | ||
): Promise<Response> { | ||
const urlString = request.url; | ||
let redirectedHeaderKey = 'simulated-fetch-redirected'; // Temporary header to track if the request was redirected in the redirection chain | ||
|
||
if (result.status >= 300 && result.status < 400) { | ||
const location = result.headers.get('location'); | ||
if (location) { | ||
request.headers.set(redirectedHeaderKey, 'true'); | ||
return await this.fetch(new URL(location, urlString), request); | ||
} | ||
} | ||
|
||
// We are using Object.defineProperty because `url` and `redirected` | ||
// response properties are read-only. We are overriding these properties to | ||
// conform to the Fetch API specification where the `url` property is set to | ||
// the final URL and the `redirected` property is set to true if the request | ||
// was redirected. Normally, when using a native fetch, these properties are | ||
// set automatically by the client, but in this case, we are simulating the | ||
// fetch and need to set these properties manually. | ||
Comment on lines
+514
to
+520
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. super useful |
||
|
||
if (request.url && !result.url) { | ||
Object.defineProperty(result, 'url', { value: urlString }); | ||
|
||
if (request.headers.get(redirectedHeaderKey) === 'true') { | ||
Object.defineProperty(result, 'redirected', { value: true }); | ||
request.headers.delete(redirectedHeaderKey); | ||
} | ||
} | ||
|
||
return result; | ||
} | ||
|
||
async fetch( | ||
urlOrRequest: string | URL | Request, | ||
init?: RequestInit, | ||
): Promise<Response> { | ||
try { | ||
for (let handler of this.urlHandlers) { | ||
let result = await handler( | ||
this.asUnresolvedRequest(urlOrRequest, init), | ||
); | ||
let request = this.asUnresolvedRequest(urlOrRequest, init); | ||
|
||
let result = await handler(request); | ||
if (result) { | ||
return result; | ||
return await this.simulateFetch(request, result); | ||
} | ||
} | ||
return await getNativeFetch()(this.asResolvedRequest(urlOrRequest, init)); | ||
|
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.
whats the 'c' in abc. Is it clearer if its node-b.ab??
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.
.abc is just a url domain I made up.