workshopper / stream-adventure

go on an educational stream adventure!
Other
2.06k stars 313 forks source link

remove request dependency #232

Closed ccarruitero closed 4 years ago

ccarruitero commented 4 years ago

request is deprecated

These dependency is used only in http_client exercise. I think we should use http.request instead

jordanSu commented 4 years ago

I think we can do that. I write the code below, and it passes the existing test.

const http = require('http');

const options = {
    hostname: 'localhost',
    port: 8099,
    method: 'POST'
}
const req = http.request(options, (res) => {
    res.on('data', (chunk) => {
        process.stdout.write(chunk);
    });
});

process.stdin.pipe(req);

I am thinking that is it possible for http module to pipe directly from stdin to stream then to stdout as below (something like process.stdin.pipe(req).pipe(process.stdout))?

Because I think it would better describe the concept of stream. If I understand the http module correctly, it seems not possible to do the same trick here. What do you think?

ccarruitero commented 4 years ago

Hi @jordanSu

Thanks for take a look into this. Do you mind to start a PR for this?

You can't pipe http.request() directly to stdout since http.request() give you a http.ClientRequest, that is a writable stream; and you need to pipe from a readable stream or a duplex. Instead you could pipe the response of the request to stdout.

const req = request('http://localhost:8099', { method: 'POST' }, (res) => res.pipe(process.stdout))

process.stdin.pipe(req)
jordanSu commented 4 years ago

Hi @ccarruitero I start a PR #236 for this issue.

Please help review it, and any advice is welcome, especially the problem description part.

ccarruitero commented 4 years ago

fixed in https://github.com/workshopper/stream-adventure/commit/938de21f89a2a26665fc471a86aaf3559302d94a