Regression in 7.0.0 options processing

[bisubus]

#217 loosened unnecessary restrictions on body option that were discussed in #212 and allowed it to be an object that is generally considered plain by a developer, i.e. not a complex object - a buffer or a stream, which are processed differently.

#297 introduced a regression that imposed a restriction on body option again.

Excessive sanitization results from the use of is-plain-obj and restricts body object prototype chain to contain only Object.prototype or null. This restriction results in counter-intuitive behaviour and requires a developer to use tricks like Object.assign({}, obj) instead of not doing them, even if an object is perceived as 'plain' (in my experience it was querystring.parse()) .

More importantly, this results in breaking changes that aren't reflected in change logs. The restrictions were dropped for a good reason, and resulting behaviour has been already relied on by package users. Now the regression just happened without an obvious reason.

[sindresorhus]

(in my experience it was querystring.parse()) .

I can't reproduce that:

isPlainObj(querystring.parse('x'));
//=> true

[bisubus]

@sindresorhus I've just checked it, and it seems that it was changed in Node 8, querystring.parse('x') directly inherits from null. isPlainObj(querystring.parse('x')) should be false in lower Node versions. This is the point where the problem becomes sinister. It's a very good thing that this issue came up before I've updated Got to 7 in the project where I actually had this case, I'd rack my brains trying to figure out why it works in one environment and throws up in another.

I'm sure this is edge case. And I think it's very illustrative of why it's may be not the best idea idea to provide breaking changes just because we can. I'm not the only user of Got, and there can be infinite amount of edge cases - I was notified of it by another user.

People have to resort to Object.assign({}, obj) hack here, and it smells. {...obj} is better, but was just added to Node, it even doesn't exist in 8.0.

[sindresorhus]

I've just checked it, and it seems that it was changed in Node 8,

I got the same result in latest Node.js 4 and 6 too.

---

Regardless, we'll consider your request to change this.

[bisubus]

@sindresorhus I wonder how did you switch between versions. I have separate node executables for that, and I've got same results in runkit, https://runkit.com/sub/59d4947b71579b001199681e and https://runkit.com/sub/59d495ca735bd50012ba27bc . I'd expect it to be that way because that's what Node sources tell.

[sindresorhus]

I used n and the Node.js REPL.

[sindresorhus]

Pull request welcome to loosen the restriction again, but should come with a test so it's not regressed in the future.