Follow redirects in the loader #1099
Conversation
| test('is able to follow redirects', async function (assert) { | ||
| loader.prependURLHandlers([ | ||
| async (request) => { | ||
| if (request.url.includes('node-b.abc')) { |
There was a problem hiding this comment.
whats the 'c' in abc. Is it clearer if its node-b.ab??
There was a problem hiding this comment.
.abc is just a url domain I made up.
| @@ -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.
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.
I think there is currently no url handler in the non-test code that would result in a redirect.
Tests use maybeHandle and that one does result in a redirect in some cases (e.g. /sth/person -> sth/person.gts when the mime type indicates code source)
There was a problem hiding this comment.
I think there is currently no url handler in the non-test code that would result in a redirect.
Not in tests, I think most redirects occur when clicking the pills. The url fetched is /sth/person. I think maybeHandle does handle that.
There was a problem hiding this comment.
Hm, I would think that in that case the native fetch is used which follows redirections automatically (by default)
| // 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. |
|
@tintinthong I see, yeah I remember you explained the reasons for mocking in #667. During our recent pairings on the loader refactor we decided it's a better idea to put that functionality right in the loader, mainly to conform to the fetch spec and that the acceptance tests don't have to know about redirections |
This PR adds an internal loader functionality where it will follow redirects of responses returned by the loader's URL handlers.
This addition has a couple of really good advantages:
response.urlneeds to contain the final url in the redirection chain, andresponse.redirectedmust betruein case of redirection)