Solving the json option confusion by replacing it with a parse option #264
Conversation
Removed the implementation. Removed option use from readme.
All these tests fail as a result of removing the json specific parse option. Each test will be reviewed and updated to work with the - to be implemented - general parse option.
Changes the json option flag into the new parse option. Does not touch tests that require other modifications or are in the to-be-replaced json test file.
This test flagged setting a body to something other than ReadableStream, string, Buffer or plain object. Since we removed the json shortcut you'll have to always stringify yourself which means you can no longer set the body to a plain object, and so we won't suggest it in our type error.
This test mainly signals the wider intention of the parse option. The other tests use the JSON variant. It also tests a common scenario that should work.
Since we are no longer sure what method of parsing the user is wanting to use we can no longer automagically set an Accept header for you.
Adds the suggested parse option. Adds a warning explaining the parse option cannot be used in combination with got's stream mode. Sets all tests to be expected to pass again.
|
Just noticed there was a misguided commit in there. Dropped it and rewrote history to improve the PR. I think we can improve the test that confused me. The details will be in the PR under this comment. |
|
Very good PR and reasoning @alextes. Sorry not getting to this sooner. I'm not entirely sold on the change itself though. The reason we added a |
|
Fair enough @sindresorhus. It's 😢 to close my first PR but I'll get over it 😉. I knew this chance as there from the beginning when I chose not to discuss my solution first. It was a fun and good way to familiarize myself with got. |
|
Yeah, always hard to close PRs that had a lot of work gone into them, but that's why opening an issue first is often beneficial. |
Solving the JSON option confusion
Preamble
You're looking at my first significant PR hooray 🎉! Got is awesome and inspired me to submit it. I’ve put a lot of thought into this PR and its code; nevertheless, the project comes first. Feel free to be completely honest. Also quite excited after spending hours creating and rebasing. Most commits have an elaborate description, I hope they will be a joy to read.
On to the PR!
Got wants to support parsing of responses and sending of encoded bodies. The current JSON option achieves one of these but seems to confuse people. After a lot of comparing libraries and thinking here’s what I believe we can do!
Summarizing from #174 and my thoughts here’s why the JSON option currently confuses:
I think from #174 the easiest solution is to bundle all parsing under one parse option and drop the confusing JSON option. To my thinking, this would lead to better expected behaviour (and is a little more flexible going forward).
But that’s not where it ends!
I'm thinking of a next PR that would add some nice default parsing based on the Content-Type header (that's what that’s for after all). Seems like a nice thing to do 😄. Might also make Got too complex, let me know what you think!
I also created #265 because - as I hinted earlier - I think the body encoding default is confusing people. I have added a few suggestions to ease use there too.