Conversation
Yeah, someday I think we can fix these warts in our API, but for now I think 2 methods is a good idea. |
transactAsync: (callback) -> | ||
@beginTransaction() | ||
try | ||
endTransaction = (fn) => (args...) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the multiple arguments needed here? Seems like you only call resolve
or reject
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess so: this is a function that returns a function (i.e. curried), so when called with endTransaction(resolve)
, for instance, will return a function that:
- Ends the transaction.
- Calls
resolve
with all the passed arguments.
When used like result.then(endTransaction(resolve))
, resolve
will be invoked with all the arguments available in then
's callback. This means that a potential client could use this API like the following:
atom.config.transactAsync(-> Promise.resolve(1)).then((n) -> console.log("Output: #{n}"))
#=> Output: 1
Although we're not using it in our production code, it seemed reasonable to follow the Promise
API semantics when I implemented this. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed the currying. So long as you test it I'm cool having the functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ I tested this behavior in c6cb37a 👍
I am going to as soon as the build gets 💚
Introduce Config::transactAsync
Nice! |
Refs: #9508
This method is supposed to wrap asynchronous code within a
Config
transaction. It accepts acallback
which must return aPromise
: when this resolves or gets rejected (or thecallback
throws an error) we complete the transaction.I am not super happy with the name I've chosen for this API, as it would have probably been better to follow Node's conventions (i.e. rename
::transact
to::transactSync
and name the new one::transact
): unfortunately that method name was already taken and it's part of the public API.Another thought was to include this feature right inside the existing
::transact
method: although possible, the resulting method would need to conform to two different contracts based on the return type ofcallback
, which feels somewhat misleading for clients. In the end having a separate method felt somewhat cleaner.Feedback super welcome! 🙇
/cc: @nathansobo @joshaber @atom/feedback