@ramimoshe Thanks a lot for your PR! This goes in the right direction and just needs a bit more work 😃

thanks,
I changed the error to be ERR_INVALID_URL, and I did a small refactor

Thanks for following up.
Now all that's left is to fix the tests:
https://github.com/nodejs/node/blob/4518d94e6104323b06781ba608e59208d35a6129/test/parallel/test-querystring-escape.js#L16
https://github.com/nodejs/node/blob/4518d94e6104323b06781ba608e59208d35a6129/test/parallel/test-url-parse-invalid-input.js#L28
https://github.com/nodejs/node/blob/4518d94e6104323b06781ba608e59208d35a6129/test/parallel/test-querystring.js#L273-L276
with something like:

common.expectsError(
  () => qs.stringify({ foo: '\udc00' }),
  {
    code: 'ERR_INVALID_URI',
    type: TypeError,
    message: 'URI malformed'
  }
);

or just as the validator for assert.throws

common.expectsError({
  code: 'ERR_INVALID_URI',
  type: TypeError,
  message: 'URI malformed'
});

I did a small refactor

👍

@refack
I am not sure how to update 'decodeURIComponent' function
so "node/test/parallel/test-querystring.js Lines 273 to 276 " test is still checking URIError

 // invalid surrogate pair throws URIError 
 assert.throws(function() { 
   qs.stringify({ foo: '\udc00' }); 
 }, /^URIError: URI malformed$/); 

StackTrace from the test (after changing):

URIError: URI malformed
    at decodeURIComponent (<anonymous>)
    at Url.parse (url.js:292:19)
    at Object.urlParse [as parse] (url.js:98:5)
    at Object.<anonymous> (/Users/rami.moshe/Projects/forks/node/test/parallel/test-url-parse-invalid-input.js:27:5)
    at Module._compile (module.js:600:30)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:521:32)
    at tryModuleLoad (module.js:484:12)
    at Function.Module._load (module.js:476:3)
    at Function.Module.runMain (module.js:641:10)

@ramimoshe you can not update the decodeURIComponent function and you should not even try. It is a native function.

The error type should definitely not be changed because it would be inconsistent in that case what error we might return. Please switch back to the URIError and just add that type to the internal/errors as I pointed out.

Ping @ramimoshe

Thank for your comments
but I am not fully understand you
@BridgeAR @refack do you mean to add "URIError" to module.exports in internal/errors?
e.g:

module.exports = exports = {
  message,
  URIError, 
  ....

or using the exported Error with URIError
e.g:

throw new errors.Error(URIError)

This is how you can add the URIError to the internal/errors.

module.exports = exports = {
  message,
  Error: makeNodeError(Error),
  TypeError: makeNodeError(TypeError),
  RangeError: makeNodeError(RangeError),
  URIError: makeNodeError(URIError), // <---- 
  AssertionError,
  E // This is exported only to facilitate testing.
};

And this is how you would use it in a different file.

const errors = require('internal/errors');
// ...
throw new errors.URIError('ERR_INVALID_URI');

I think I have brought this up in a URL PR before but couldn't find the link. The ECMAScript spec says URIError should only be raised by built-in functions, i.e. functions like decodeURIComponent() that are implemented as part of the language, we should not raise that in our codebase anyway.

(Going to board so no time to dig the link to spec)

I couldn't find a reference in the ECMAScript spec (image because the spec takes a while to load)

The Web IDL spec only excludes SyntaxError - https://heycam.github.io/webidl/#idl-exceptions

@ramimoshe, sorry for the delay.

so "node/test/parallel/test-querystring.js Lines 273 to 276 " test is still checking URIError

Use common.expectsError, specifying the type and code. Example

@refack IMO

Indicates that one of the global URI handling functions was used in a way that is incompatible with its definition.

means it excludes user land methods from throwing a URIError, because those are not "global URI handling functions". Also the WHATWG URL doesn't throw URIError either, it throws TypeError in similar situations.

Anyway, I am still fine with throwing URIError in this PR because that's what we do at the moment.

Also, test/parallel/test-querystring-escape.js needs to be updated as well.

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

@joyeecheung do you know the issue with ubuntu1404-64 build?
e.g: https://ci.nodejs.org/job/node-test-commit-linux/13309/nodes=ubuntu1404-64/console

@ramimoshe Nope, looks like a flake because the failing test should not go through the query string module.

@joyeecheung do i have to do something or just wait to approving?

@ramimoshe This is semver-major so needs at least 2 TSC approvals... @nodejs/tsc

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/11040/

https://ci.nodejs.org/job/node-test-linter/13033/console

not ok 11 - /usr/home/iojs/build/workspace/node-test-linter/lib/internal/errors.js
  ---
  message: '"ERR_INVALID_URI" is not documented in doc/api/errors.md'
  severity: error
  data:
    line: 291
    column: 1
    ruleId: documented-errors
  messages:
    - message: doc/api/errors.md does not have an anchor for "ERR_INVALID_URI"
      severity: error
      data:
        line: 291
        column: 1
        ruleId: documented-errors
  ...

@ramimoshe Do you have time to document this error code? If not I can do that, just don't want to delay this PR for too long.

@joyeecheung - yes, i will fix it

@ramimoshe Thanks!