Use individual args and validate them.

[boennemann]

An options object suggest that some arguments might not be required.
In the case of this module all arguments are required and not passing any of them leads to very unexpected and dangerous results.

Throws an Error when arguments are missing or of the wrong type, instead of ignoring or coercing.

[gr2m]

I don’t agree with this, options are much more readable and self-explanatory, you don’t need to remember the order of arguments. And I’d argue that this is so low level and that it might be used excessively, so validation might be performance impact

[boennemann]

This is not a matter of style or readability, but semantics. Options are modifiers to a certain operation.

a thing that is or may be chosen.

They are by definition optional and have a default that's going to happen when they aren't passed.

In this case we're looking at a function that converts a given input to a certain output. None of the arguments are optional, or have any defaults, in order for them to generate the correct output.

I agree that an options object is a way better choice if there are indeed options.

// bad API
PouchDB.replicate('a', 'b', true, false)
// good API
PouchDB.replicate('a', 'b', {
  live: true, 
  retry: false
})

// bad API
document.addEventListener('click', something, false)

This function is low level and critical for the operation it's going to be used for. That's exactly why it should provide the best error output possible, so users can catch potentially fatal mistakes. As it's the nature of hashing the output of this function doesn't give a human any clue to what could possibly be wrong. With the current implementation this is possible:

generateSessionID({timestamp: [], salt: {a: 1}})
// 'dW5kZWZpbmVkOjpr9KGRZVIFE_X2OK54FmNZH1xa3A'

In alignment with the UNIX philosophy this module should be doing exactly one thing, and it should be doing it well. That's why I would want to use a module for this kind of operation, so I don't have to care for all the edge-cases myself. I wouldn't describe the above behaviour as doing this well.

The aproba module is as simple and performant as it can get: https://github.com/iarna/aproba/blob/master/index.js I wouldn't make perf a concern unless it's really the bottleneck of an application.

[boennemann]

To add to this: With the options object a simple typo leads to fatal results:

// secret misspelt
var sessionId = generateSessionId({
  username: 'pat',
  salt: '24eb90e9e1343977b8323857287ffca4',
  sercet: '78875068a1979fb910d5d8f37d316aa4',
  timestamp: 1449689785
})

// expected:  'cGF0OjU2Njg4MkI5OuY74QyC7HgrKeFv8sWuds_vNfRC'
// actual:    'cGF0OjU2Njg4MkI5OnxG0uI0oo54cXFuU8heYXgxse7s'

[gr2m]

Thanks for the write up @boennemann, that is very helpful! I'm still not convinced though, it’s to easy to make the mistake of

generateSessionId(username, secret, salt, timestamp)

although salt should come 2nd, and secret 3rd.

You seem much more passionate about it than me though :) So I’d suggest, if there is no other opinion by next week, let’s merge it

[janl]

What @boennemann says +1 :)

We could argue about timestamp being required, we could generate that inside the fun, so we are down to username, salt and secret and then good naming can help, how about this?

function generateSessionId(userName, userSalt, serverSecret) {}

[boennemann]

According to @janl's suggestion I updated the argument names now, to make things even more unambiguous. I still kept the timestamp argument though, because I wanted the function to remain pure.

I agree that it's easy to mix up argument order, but (without further data to back this) I think it mostly happens when it's a user-facing API that's going to be used over and over again. Instead of looking up the documentation, or directly copying the example people try and start to remember things, and that's where the mistake happens. This is probably not the case for this function.

Also this is what my editor gave me without any extra config:

Tools can take care of it. (The slightly messed up type annotations only call for more validation ;))

[gr2m]

👍