Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SlackStore (replacement for DataStore) #410

Closed
3 tasks done
aoberoi opened this issue Oct 3, 2017 · 7 comments
Closed
3 tasks done

SlackStore (replacement for DataStore) #410

aoberoi opened this issue Oct 3, 2017 · 7 comments
Labels
enhancement M-T: A feature request for new functionality

Comments

@aoberoi
Copy link
Contributor

aoberoi commented Oct 3, 2017

Description

For reasons discussed in #330, there is a need to provide a better alternative to the SlackDataStore abstraction. This issue is meant to help specify a new solution and to scope the first implementation.

  1. The API for storing and fetching data must be asynchronous. Ideally, this means both with callbacks and with Promises. (Support for a data store that is implemented via promises #246).

  2. The API must allow processing of incoming events from the RtmClient of the @slack/client package AND from the SlackEventAdapterof the @slack/events-api package.

    • De-duplication of incoming events is not guaranteed if sourced from an RtmClient, but must be guaranteed if sourced from a SlackEventAdapter. It must be very clear that de-duplication isn't being handled, possible even an error, if one store is connected to more than one RtmClient.
  3. The API for storing and fetching data should be optimized for both relational backends and key-value backends. This might mean that there are two separate interfaces that a backend can choose between implementing.

  4. The API should make it trivial to update the data using outgoing requests. For example, the WebClient can be used to call the users.list method, and it should be trivial to update the data in the store with the results.

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@aoberoi aoberoi added the enhancement M-T: A feature request for new functionality label Oct 3, 2017
@aoberoi
Copy link
Contributor Author

aoberoi commented Oct 3, 2017

another use case to possibly incorporate: #306. it seems the datastore wasn't exactly designed well to handle bot users. we should help developers reason about the differences between Bot IDs and Bot User IDs. we may also need to think about how this works in an XOXA token world (Workspace Token Apps).

@aoberoi
Copy link
Contributor Author

aoberoi commented Oct 3, 2017

another requirement: disambiguate private groups from MPIMs (or MPDMs). handle the G->C conversion problems.

@clavin
Copy link
Contributor

clavin commented Oct 29, 2017

Ideally, this means both with callbacks and with Promises.

I don't like this, not one bit. It feels completely redundant to have both. I opt for choosing and sticking with Promises, but that's just a blind choice based on my personal preference. Nonetheless, I don't see a reason to support both.

@aoberoi
Copy link
Contributor Author

aoberoi commented Oct 30, 2017

@clavin thanks for speaking up!

while i personally feel fluent in both reading and writing code with Promises, i'm still not sure about the larger node and JavaScript community. i get the feeling that the majority of developers are still unfamiliar with (and aren't interested in learning to write) promise-based code.

this motivates me to think that if we had to pick one, it would probably be callbacks. that goes against my personal preference, but if it was callbacks or both, which would you choose?

PS. i know about util.callbackify(), but that's only available since node v8.2.0, and i assume the audience that's not interested in adopting promises is also mostly not building on the latest versions of node.

@clavin
Copy link
Contributor

clavin commented Oct 30, 2017

Ah, I guess it's just me blinding myself. I was (and still am) under the illusion that Promises have been widely well-received and commonplace; I didn't realize that the wider Node/JS community might not really be interested/comfortable. In that case, I say go all out with a duplex asynchronicity implementation (both promises and callbacks) to make happy as many people as possible.

As an added benefit, I can see that having both means:

  • Attraction to well-rounded Node/JS devs who are comfortable with the latest technology, including promises
    • i.e. easy to work with in async-await environments
  • Attraction to newer JS devs who are just barely grasping the ropes of asynchronicity
    • And then there's also people who live in a world without the graciousness of promises on purpose (maybe due to compatibility with past versions of node)

This is only a good thing, however, if your target audience for the SDK includes not-so-versed/past-dwelling developers.

Also, I was a little ambiguous (my fault) in my comment; I've continued to feel that the public interaction from SDK-consumer (a developer) to SlackStore should totally be versatile in order to adapt to all types of developers. Anyone who wants to use the SDK overall should be able to do so with ease, and having both promises and callbacks is a pretty amazing way of making the SDK easy to use for all.

I was more-so caught up in the implementation details, wherein I felt that it would be weird having a system where the internal back-end for handling data storage has both promises and callbacks. That is, making available to those who implement storage back-ends, i.e. Redis or in-memory or file system or SQL, both implementations of asynchronicity would be a weird choice. A bit of a niche complaint, though. I suppose it's better to support both for internal actions too simply for widening the range of support for the SDK.

All in all, as long as the details of implementation are figured out, I'm happy. Regardless of what they are, figuring out the details paves the way for actually implementing those details, leading to an amazing, new SDK ✨😎. Can't wait to see what's in store 👍.

@aoberoi
Copy link
Contributor Author

aoberoi commented Jan 5, 2018

found a neat library that has the potential to power part of this solution: https://github.com/lukechilds/keyv

@aoberoi aoberoi changed the title SlackStore (replacement for Data Store) SlackStore (replacement for DataStore) Apr 27, 2018
@aoberoi
Copy link
Contributor Author

aoberoi commented Aug 28, 2020

Thanks for all the discussion everyone! The platform and the state of our SDKs and tools have changed quite a bit on the last few years. At this point, it doesn't seem helpful to pursue this feature in this SDK.

If you're looking for an even easier time building with this SDK, then I suggest you check out Bolt for JS. It provides a simpler API to listen to events and store the bits of data that your app needs to run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality
Projects
None yet
Development

No branches or pull requests

2 participants