Node: Validate API options

Created on 18 Dec 2018 · 7Comments · Source: nodejs/node

There is an ongoing effort to validate our API input stricter. As such we add more and more type checks for the options. One thing we still mainly lack is checking if there's a typo in an option. This could easily be done by checking if the passed in option is known or unknown. Obviously this would be semver-major, just as any other added type check.
Doing this for all APIs is a lot of work and can only be done iterative.

I would like to start with util.inspect but I don't really see any rush to do this. If we all agree on doing this everywhere throughout the code base, this could be a good first contribution and a good task for a code and learn event.

Refs: https://github.com/nodejs/node/pull/24971#issuecomment-448043734

// CC: @nodejs/tsc @joyeecheung

File tracking:

lib:

[ ] assert.js
[ ] async_hooks.js
[ ] buffer.js
[ ] child_process.js
[ ] cluster.js
[ ] console.js
[ ] constants.js
[ ] crypto.js
[ ] dgram.js
[ ] dns.js
[ ] domain.js
[ ] events.js
[ ] fs.js
[ ] http2.js
[ ] _http_agent.js
[ ] _http_client.js
[ ] _http_common.js
[ ] _http_incoming.js
[ ] http.js
[ ] _http_outgoing.js
[ ] _http_server.js
[ ] https.js
[ ] inspector.js
[ ] module.js
[ ] net.js
[ ] os.js
[ ] path.js
[ ] perf_hooks.js
[ ] process.js
[ ] punycode.js
[ ] querystring.js
[ ] readline.js
[ ] repl.js
[ ] _stream_duplex.js
[ ] stream.js
[ ] _stream_passthrough.js
[ ] _stream_readable.js
[ ] _stream_transform.js
[ ] _stream_wrap.js
[ ] _stream_writable.js
[ ] string_decoder.js
[ ] sys.js
[ ] timers.js
[ ] _tls_common.js
[ ] tls.js
[ ] _tls_wrap.js
[ ] trace_events.js
[ ] tty.js
[ ] url.js
[ ] util.js
[ ] v8.js
[ ] vm.js
[ ] worker_threads.js
[ ] zlib.js

lib/internal:

[ ] assert.js
[ ] async_hooks.js
[ ] bash_completion.js
[ ] buffer.js
[ ] child_process.js
[ ] cli_table.js
[ ] cluster
[ ] constants.js
[ ] dgram.js
[ ] domexception.js
[ ] encoding.js
[ ] error-serdes.js
[ ] errors.js
[ ] fixed_queue.js
[ ] freelist.js
[ ] http.js
[ ] inspector_async_hook.js
[ ] linkedlist.js
[ ] net.js
[ ] options.js
[ ] per_context.js
[ ] print_help.js
[ ] priority_queue.js
[ ] querystring.js
[ ] queue_microtask.js
[ ] readline.js
[ ] readme.md
[ ] repl.js
[ ] safe_globals.js
[ ] socket_list.js
[ ] stream_base_commons.js
[ ] timers.js
[ ] tls.js
[ ] trace_events_async_hooks.js
[ ] tty.js
[ ] url.js
[ ] util.js
[ ] v8_prof_polyfill.js
[ ] v8_prof_processor.js
[ ] validators.js
[ ] worker.js
[ ] wrap_js_stream.js

lib/internal/bootstrap:

[ ] cache.js
[ ] loaders.js
[ ] node.js

lib/internal/cluster:

[ ] child.js
[ ] master.js
[ ] round_robin_handle.js
[ ] shared_handle.js
[ ] utils.js
[ ] worker.js

lib/internal/console:

[ ] constructor.js
[ ] global.js
[ ] inspector.js

lib/internal/crypto:

[ ] certificate.js
[ ] cipher.js
[ ] diffiehellman.js
[ ] hash.js
[ ] keygen.js
[ ] pbkdf2.js
[ ] random.js
[ ] scrypt.js
[ ] sig.js
[ ] util.js

lib/internal/dns:

[ ] promises.js
[ ] utils.js

lib/internal/fs:

[ ] promises.js
[ ] read_file_context.js
[ ] streams.js
[ ] sync_write_stream.js
[ ] utils.js
[ ] watchers.js

lib/internal/http2:

[ ] compat.js
[ ] core.js
[ ] util.js

lib/internal/modules/cjs:

[ ] helpers.js
[ ] loader.js

lib/internal/modules/esm:

[ ] create_dynamic_module.js
[ ] default_resolve.js
[ ] loader.js
[ ] module_job.js
[ ] module_map.js
[ ] translators.js

lib/internal/process:

[ ] coverage.js
[ ] esm_loader.js
[ ] main_thread_only.js
[ ] next_tick.js
[ ] per_thread.js
[ ] promises.js
[ ] stdio.js
[ ] warning.js
[ ] worker_thread_only.js
[ ] write-coverage.js

lib/internal/repl:

[ ] await.js
[ ] recoverable.js

lib/internal/streams:

[ ] async_iterator.js
[ ] buffer_list.js
[ ] destroy.js
[ ] duplexpair.js
[ ] end-of-stream.js
[ ] lazy_transform.js
[ ] legacy.js
[ ] pipeline.js
[ ] state.js

lib/internal/test:

[ ] binding.js
[ ] heap.js
[ ] unicode.js

lib/internal/util:

[ ] comparisons.js
[x] inspect.js https://github.com/nodejs/node/pull/24971
[ ] inspector.js
[ ] types.js

lib/internal/vm:

[ ] source_text_module.js

All 7 comments

If I understand this right, that means any unrelated object properties would cause exceptions to be thrown. If so, I'm very much -1 on that. We should only be caring about properties we expect. Forcing people to create new objects or delete existing properties just to prevent exceptions from node core isn't a good policy IMO.

mscdex on 18 Dec 2018

👍1

@mscdex I don't really want this on all APIs. It is likely a case to case situation but a lot of our APIs only accept a specific set of options and adding others are silently ignored without any further effect. Those that are ignored are very likely typos and we are able to provide an improved user experience that way.

I would ask to first have a look at the existing API and the current implementation and to decide afterwards if it makes sense or not.

BridgeAR on 18 Dec 2018

breakage_everywhere

Yeah .. nah .. this is going to hurt a bit too much I think. I'd love if this had been done from the beginning, but Node was born in the jQuery era when we were all infatuated with how magical overloading and coercion could be. This ship has sailed and we're better off doing what we can to build guards around the implicit flexibility our API offers.

rvagg on 18 Dec 2018

@rvagg if we do this API by API and check CITGM each time (which should be done for every semver-major) we should be able to limit the actual breakage to a pretty small amount. Don't you think so?

BridgeAR on 18 Dec 2018

Nope, citgm is a teeny tiny snapshot of the ecosystem, and I doubt many of them have 100% coverage either. There's an absolute _ton_ of Node code that will never see the light of day, that we'll never get to test or grep, in organisations large and small that build critical applications on Node and make implicit assumptions about our API because _it just works_.

rvagg on 18 Dec 2018

👍1

I wouldn't like to see this. I find it very useful to have an object with properties, and to pass it to multiple APIs, each of them picking up the properties they need.

The idea of doing it for only some APIs is even worse, IMO, than doing it for all. Our API's behaviour would become the subject of "guess which of these APIs is different" trivia quizs.

It's a bit like the object own-property debate... if I can do console.log(opts.shell); and see my shell, I should be able to do child_process.exec(opts) and have it respect the shell property. Its exactly what any javascript developer would expect.

Similarly, I don't think js devs expect APIs to explode because there are extra properties.

Can't people (optionally) use .ts annotations if for some reason they want this strict/fragile behaviour?