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

Context method updateConversation should accept expiration time #190

Closed
3 tasks done
aoberoi opened this issue May 14, 2019 · 3 comments
Closed
3 tasks done

Context method updateConversation should accept expiration time #190

aoberoi opened this issue May 14, 2019 · 3 comments
Assignees
Labels
enhancement M-T: A feature request for new functionality good first issue Good for newcomers
Milestone

Comments

@aoberoi
Copy link
Contributor

aoberoi commented May 14, 2019

Description

The conversationContext() built-in middleware, which is added to apps by default, is meant to add a function into the context called updateConversation(state). It allows listeners and middleware to store state that is contextual to the conversation (channel, im, mpim, or group) for use during the handling of a subsequent event.

The App option convoStore allows the user to choose a specific storage provider, as long as it conforms to the ConversationStore interface.

The ConversationStore interface defines the behavior of the set() method such that it can accept an expiration time (expiresAt) in milliseconds. The default implementation, MemoryStore, uses this argument to ensure that subsequent calls to get() don't return the value if it is expired.

The hole here is that updateConversation() doesn't allow the listener or middleware to set an expiration time for the state. It should accept the expiration time as an additional parameter.

Open question: should the additional parameter of updateConversation() be an absolute time (like number of milliseconds since unix epoch), or a relative time (like the number of milliseconds into the future)? I imagine that most developers will have to do some Date math to express a relative time in terms of absolute time, and in that case accepting the relative time directly can offer meaningful convenience.

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 May 14, 2019
@seratch seratch added the good first issue Good for newcomers label Mar 19, 2021
@seratch
Copy link
Member

seratch commented Mar 19, 2021

This issue is a very specific task to update this logic: https://github.com/slackapi/bolt-js/blob/%40slack/bolt%403.3.0/src/conversation-store.ts#L61-L62

@shubhamjajoo
Copy link
Contributor

HI @seratch ,
I have raised PR for this issue here

@seratch
Copy link
Member

seratch commented Nov 30, 2021

Fixed by #1221

@seratch seratch closed this as completed Nov 30, 2021
@seratch seratch added this to the 3.9.0 milestone Nov 30, 2021
@seratch seratch self-assigned this Dec 16, 2021
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 good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants