automerge / automerge-classic

A JSON-like data structure (a CRDT) that can be modified concurrently by different users, and merged again automatically.
http://automerge.org/
MIT License
14.75k stars 466 forks source link

dev UX: Automerge.Frontend.change argument order #399

Closed okdistribute closed 3 years ago

okdistribute commented 3 years ago

Right now, there is an optional argument 'message' which can be passed in as the second argument to Automerge.Frontend.change

function change<D, T = Proxy<D>>(doc: D, message: string | undefined, callback: ChangeFn<T>): [D, Change]
function change<D, T = Proxy<D>>(doc: D, callback: ChangeFn<T>): [D, Change]

Usually, libraries put optional parameters at the end of the function parameter list to ensure that calls across the app are similar and don't require special logic around the order of parameters. Ideally, the signature for the overloaded function would be:

 function change<D, T = Proxy<D>>(doc: D, callback: ChangeFn<T>, message?: string): [D, Change]
ept commented 3 years ago

Hi @okdistribute, thanks for the feedback. I realise optional arguments usually go at the end, but we chose this argument order because we thought it was easier to read this:

newDoc = Automerge.change(oldDoc, 'my message', doc => {
  // ... several lines of stuff ...
})

than this:

newDoc = Automerge.change(oldDoc, doc => {
  // ... several lines of stuff ...
}, 'my message')

since in the second example the message appears somewhat awkwardly stuck at the end of the function closure. Perhaps this is also my Ruby background shining through — in Ruby, a callback passed to a function (a block argument) is always the last function argument. But I realise that such matters of aesthetics are always up for debate, and JS is not Ruby.

Does anyone else have opinions on this matter? I'm up for changing the argument order if there is sufficient demand, but we'd have to do it before 1.0 since it would be a breaking change to the API.

okdistribute commented 3 years ago

Ah I see. That makes a lot of sense!

Well, perhaps it feels a little different in this case because even though it is a function passed to Automerge, it is a synchronous call. Not sure what to do, it is a little awkward either way so keeping it as is may be just fine.

ept commented 3 years ago

Okay. On balance I think it's probably best to keep it as-is, but thanks again for your suggestion.