Skip to content
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

Add options to response #188

Closed
wants to merge 1 commit into from
Closed

Add options to response #188

wants to merge 1 commit into from

Conversation

luanmuniz
Copy link
Contributor

Hi. I'm not sure if im doing it the correct way or if you want this in the repo, but this is the solution i found to he following problem:

I have a promise code like this:

requestsPromise = [];
PackofURL.forEach((thisUrl) => {
    var thisRequest = got(thisUrl.url);
    requestsPromise.push(thisRequest);
});

return Promise.all(requestsPromise)
    .then((allRequests) => {
        allRequests.forEach((thisRequest, index) => {
            if(CheckURLForSomething(thisRequest)) {
                Requester.doSomething(thisRequest);
            }
        });
    });

If the request returns 200 i pass it into CheckURLForSomething and it will hash the url to store as a key of another part of the code.
The problem is that i could not build the full URL (protocol + host + path + query) with the response.

I could do something like this:

PackofURL.forEach((thisUrl) => {
    var thisRequest = got(thisUrl.url)
        .then((response) => { return Promise.resolve({ fullUrl: thisUrl.url, response: response }); });

    requestsPromise.push(thisRequest);
});

return Promise.all(requestsPromise)
    .then((allRequests) => {
        allRequests.forEach((thisRequest, index) => {
            if(CheckURLForSomething(thisRequest.fullUrl)) {
                Requester.doSomething(thisRequest.response);
            }
        });
    });

But maybe more people have the same problem and with this change i can get just thisRequest.request.href.
If you want this in the code, let me know if i should change anything.

Hi. I'm not sure if im doing it the correct way or if you want this in the repo, but this is the solution i found to he following problem:

I have a promise code like this:
```
requestsPromise = [];
PackofURL.forEach((thisUrl) => {
	var thisRequest = got(thisUrl.url);
	requestsPromise.push(thisRequest);
});

return Promise.all(requestsPromise)
	.then((allRequests) => {
		allRequests.forEach((thisRequest, index) => {
			if(CheckURLForSomething(thisRequest)) {
				Requester.doSomething(thisRequest);
			}
		});
	});
```
If the request returns `200` i pass it into `CheckURLForSomething` and it will hash the url to store as a key of another part of the code.
The problem is that i could not build the full URL (protocol + host + path + query) with the response.

I could do something like this:
```
PackofURL.forEach((thisUrl) => {
	var thisRequest = got(thisUrl.url)
		.then((response) => { return Promise.resolve({ fullUrl: thisUrl.url, response: response }); });

	requestsPromise.push(thisRequest);
});

return Promise.all(requestsPromise)
	.then((allRequests) => {
		allRequests.forEach((thisRequest, index) => {
			if(CheckURLForSomething(thisRequest.fullUrl)) {
				Requester.doSomething(thisRequest.response);
			}
		});
	});
```

But maybe more people have the same problem and with this change i can get just `thisRequest.request.href`.
If you want this in the code, let me know if i should change anything.
@floatdrop
Copy link
Contributor

Hi @luanmuniz. Putting options in result can cause troubles, because this will force Node to keep body property in memory until all requests are processed. For example, if you sending many files to server with serial – you will run out of memory.

Code you provided is a way to go, but could be a little improved:

requestsPromise = PackofURL.map(thisUrl =>
    got(thisUrl.url)
        .then(response => {fullUrl: thisUrl.url, response: response})
);

return Promise.all(requestsPromise)
    .then(allRequests => {
        allRequests.forEach((thisRequest, index) => {
            if(CheckURLForSomething(thisRequest.fullUrl)) {
                Requester.doSomething(thisRequest.response);
            }
        });
    });

@luanmuniz
Copy link
Contributor Author

@floatdrop Its only a few more variables. In this PR at least, its not the http request itself.

Doesn't Node need to keep body in memory until i use it anyway? Add a few more variables should not be a problem. Can you explain better how adding those properties can cause a problem, or give me a few directions do i can search and study this better?

btw: thanks for the suggestion, i will implement it! i always forget map :(

@sindresorhus
Copy link
Owner

@floatdrop Not sure it's worth doing, but we could expose just the URL on the result, not the rest of the options.

@luanmuniz
Copy link
Contributor Author

@sindresorhus This is the opts object:

{ protocol: 'http:',
  path: '/',
  retries: [Function: backoff],
  slashes: true,
  auth: null,
  host: 'luanmuniz.com.br',
  port: null,
  hostname: 'site.com',
  hash: null,
  search: null,
  query: null,
  pathname: '/',
  href: 'http://site.com/',
  headers:
   { 'user-agent': 'got/6.2.0 (https://github.com/sindresorhus/got)',
     'accept-encoding': 'gzip,deflate' },
  method: 'GET' }

maybe href, method and headers? The rest we can build with something like urlparser form the href.

What the name of the variables should be? Just say the word and i'll update the PR

@floatdrop
Copy link
Contributor

#191 looks like a good edge case for this PR – what should be in res.request, when redirect encountered?

@luanmuniz
Copy link
Contributor Author

@floatdrop good question. I think the original request since it was the request that was made.
What do you think about it?

@sindresorhus
Copy link
Owner

For simplicity we could add a requestUrl property that is the initial URL as string. So #191 would add .url which is the final resolved URL and requestUrl would be the initial URL. What do you think about that @floatdrop?

@floatdrop
Copy link
Contributor

@sindresorhus 👍

@sindresorhus
Copy link
Owner

Cool.

@luanmuniz Can you update this PR accordingly (and mention it in the docs)?

@luanmuniz
Copy link
Contributor Author

I couldn't update this because the repository doesn't exist anymore. The updated PR is in #205
Sorry about that.

We should close this PR e continue in #205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants