dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.37k stars 9.99k forks source link

Recursive fetch doesn't work on server-side #5168

Closed rosieks closed 5 years ago

rosieks commented 7 years ago

I tried to run such code on server-side but for some reason it doesn't work. fetch function is imported from domain-task

return fetch('https://jsonplaceholder.typicode.com/posts/1')
    .then(response => response.json())
    .then(response => {
        return fetch('https://jsonplaceholder.typicode.com/posts/2')
            .then(response2 => response2.json())
    });

I receive the following error:

Exception: Call to Node module failed with error: Error: 
When running outside the browser (e.g., in Node.js), you must specify a base URL
before invoking domain-task's 'fetch' wrapper.
Example:
import { baseUrl } from 'domain-task/fetch';
baseUrl('http://example.com'); // Relative URLs will be resolved against this
SteveSandersonMS commented 7 years ago

If you want server-side prerendering to wait for your chain of promises to complete, then you must add the outer promise to the list of domain tasks, like the WeatherForecasts.ts example does in the ReactRedux template.

For example,

import { fetch, addTask } from 'domain-task';

... and then...

const myTask = fetch('https://jsonplaceholder.typicode.com/posts/1')
    .then(response => response.json())
    .then(response => {
        return fetch('https://jsonplaceholder.typicode.com/posts/2')
            .then(response2 => response2.json())
    });
addTask(myTask); // Here's where we register this task so that prerendering waits for it
return myTask;

This should keep the domain context open for as long as your promise chain is in flight, so it should not clear out the baseUrl context data before it's finished.

rosieks commented 7 years ago

I was aware of addTask from WeatherForecasts.ts - I have made a few other tests and I noticed that:

rosieks commented 7 years ago

@SteveSandersonMS - I have published example that shows this issue. To reproduce that:

SteveSandersonMS commented 7 years ago

@rosieks Thanks - I'll check it soon. Reopening.

DanHarman commented 7 years ago

@SteveSandersonMS Would you mind making addTask return the task fluently as then we can condense code like the above (of which I have a fair amount) to return addTask(myTask);

Great use case for using async await too, although not sure on the projects policy on that.

rosieks commented 7 years ago

I just made another test - I replaced first fetch with: const delay = timeout => new Promise((resolve, reject) => setTimeout(resolve, timeout)); and it also failed. It looks like baseUrl() doesn't work with promises in general.

Maybe replacing it to async/await will help?

exera commented 7 years ago

any workaround for this? it looks like when nested fetch is called there is no active domain and baseUrl returns undefined.

SteveSandersonMS commented 5 years ago

Sorry this never got a timely response.

I think the behavior reported here comes down to a Node issue whereby it fails to preserve domain context in native Promise continuations. That's not something we can directly fix, so if you're chaining promises and using addTask, I think you'll unfortunately have to re-assign the baseUrl at the start of each continuation (e.g., by calling baseUrl('http://example.com')).

@mkArtakMSFT Propose we close this as it's an external (Node) bug that we're not planning to work around.