I just ran some quick benchmarks and it seems that using the String() constructor is currently significantly slower on objects than simply calling its .toString(). It would need further benchmarking, but perhaps putting an extra conditional to avoid that particular case might help things.

@mscdex rewritten with typecheck

Not quite.

const x = Symbol('test');
'' + x; // throws
String(x) // 'Symbol(test)'

@jacobp100 the same with encodeURIComponent:

> encodeURIComponent(Symbol('test'))
TypeError: Cannot convert a Symbol value to a string

> '' + Symbol('test')
TypeError: Cannot convert a Symbol value to a string

so the current solution is better than String constructor. Thank you for additional test case :)

You’re correct, good catch. Just checked, per spec, ToString should throw for symbols.

LGTM

Are there any cases we can test where this would make a difference even if you are not setting toString()/valueOf()?

@Fishrock123 What do you mean? There will always be a toString() of some kind, whether you explicitly set one or not (if you don't, Object.prototype.toString() will get used).

Nah, he's right. If toString is not callable, then you need to try valueOf. If that's also not callable, you need to throw an error.

Fixed. + more tests

Seems like it is better to use String constructor instead all of these checks. What do you think about last commit, @mscdex?

@mscdex @nodejs/ctc ... ping

@mscdex your variant will fail for this test

// toString is not callable, must throw an error
assert.throws(() => qs.escape({toString: 5}));

and in my version there is no problem with encoding symbols - it will throw as with encodeURIComponent

LGTM with one minor nit

CI: https://ci.nodejs.org/job/node-test-pull-request/2057/

One failure in CI that looks unrelated.

@mscdex ... any further comments on this one?

@silentroach ... can you update the commit log to follow our style guidelines here: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

@jasnell Nope, other than commit message needs to be formatted correctly

@jasnell, @mscdex fixed. Is it ok now?

Yep, LGTM

New CI, just to be extra careful ;-) https://ci.nodejs.org/job/node-test-pull-request/2244/

Landed in 5dafb43

Thank you :}