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

Ignore JSON option when using got.stream() #541

Merged
merged 2 commits into from
Aug 2, 2018

Conversation

szmarczak
Copy link
Collaborator

No description provided.

@szmarczak szmarczak changed the title Override JSON option when using got.stream() Ignore JSON option when using got.stream() Aug 2, 2018
@@ -137,7 +137,7 @@ Returns a `Stream` instead of a `Promise`. This is equivalent to calling `got.st

Type: `string` `Buffer` `stream.Readable` [`form-data` instance](https://github.com/form-data/form-data)

*This is mutually exclusive with stream mode.*
*If you provide this option, `got.stream()` will be read-only.*
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this related to the JSON change and what does read-only mean here?

Copy link
Collaborator Author

@szmarczak szmarczak Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK form option is converted to body option here. read-only means the stream is no longer writable.

The only related thing is that I've changed the messages about streams.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just changed the sentence to make things clearer. The behaviour of form option remains the same.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you changed the meaning. Previously it said that the option cannot be used with got.stream() at all. Now you're saying that it can, but that the stream will be read-only. If this is true, can you point to a test proving that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously it said that the option cannot be used with got.stream() at all.

Yeah, but practically it wasn't true.

can you point to a test proving that?

Sure :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you go:

got/test/stream.js

Lines 62 to 66 in b504346

test('throws on write to stream with body specified', t => {
t.throws(() => {
got.stream(s.url, {body: 'wow'}).end('wow');
}, 'Got\'s stream is not writable when the `body` option is used');
});

See? I hope it proves it :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great :)

@sindresorhus sindresorhus merged commit 4d92eb6 into sindresorhus:master Aug 2, 2018
@szmarczak szmarczak deleted the json-patch branch January 17, 2019 18:54
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

2 participants