Issue 284 regex events #763

pdontha · 2021-01-21T00:41:16Z

Summary

Fixed #284: Added support to events to handle regex.

Requirements (place an `x` in each `[ ]`)

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.

CLAassistant · 2021-01-21T00:41:20Z

All committers have signed the CLA.

mwbrooks · 2021-01-21T01:00:58Z

Hi @pdontha 👋🏻 thanks for the pull request and welcome as a first-time contributor to Bolt JS!

I've added a few reviewers to this pull request who can work with you to merge and release your work.

In the meantime, can you please sign our CLA. You can do this by selecting the link in the CLAssistant message and signing the CLA with the same email address associated with your commits.

I've also noticed that our CI is failing due to linting errors. Would you mind running npm test (or npm run lint) in the project on your local machine to update the syntax? We use Prettier and it should guide you through any fixes. You can also install the Prettier code editor extension to make life a little easier.

Thanks again for the pull request!

codecov · 2021-01-21T01:05:52Z

Codecov Report

Merging #763 (3a4eef8) into main (5b96fb7) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
+ Coverage   65.24%   65.47%   +0.22%     
==========================================
  Files          11       11              
  Lines        1082     1089       +7     
  Branches      320      323       +3     
==========================================
+ Hits          706      713       +7     
  Misses        317      317              
  Partials       59       59

Impacted Files	Coverage Δ
src/App.ts	`80.60% <100.00%> (ø)`
src/middleware/builtin.ts	`83.72% <100.00%> (+0.93%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b96fb7...0c81c26. Read the comment docs.

pdontha · 2021-01-21T01:08:59Z

Hi @pdontha 👋🏻 thanks for the pull request and welcome as a first-time contributor to Bolt JS!

I've added a few reviewers to this pull request who can work with you to merge and release your work.

In the meantime, can you please sign our CLA. You can do this by selecting the link in the CLAssistant message and signing the CLA with the same email address associated with your commits.

I've also noticed that our CI is failing due to linting errors. Would you mind running npm test (or npm run lint) in the project on your local machine to update the syntax? We use Prettier and it should guide you through any fixes. You can also install the Prettier code editor extension to make life a little easier.

Thanks again for the pull request!

Thank you! I have signed the CLA with the correct email address. All fixed now.

aoberoi · 2021-01-25T20:55:18Z

src/App.ts

  ): void {
-    this.listeners.push([onlyEvents, matchEventType(eventName), ...listeners] as Middleware<AnyMiddlewareArgs>[]);
+    const eventMiddleware = patternsOrMiddleware.map((patternOrMiddleware) => {


is there any limit to the number of event name or patterns that can precede the listener function? for example, should we allow the following?

app.event('app_home_opened', 'app_mentioned', async () => { /* ... some listener code ... */ });

the way the PR is currently written, this call would be valid, but what does it mean? is it meant to allow the listener to be called when either an app_home_opened event or an app_mentioned event is triggered? let's take a look at what this set of arguments expands into for the line below, where the listeners are being stored.

this.listeners.push([ onlyEvents, matchEventType('app_home_opened'), matchEventType('app_mentioned'), async () => { /* ... some listener code ... */ } ]);

now its a little easier to see how an incoming event is eventually makes its way to the listener. the event is processed through each middleware in the array, starting from the beginning. onlyEvents will filter out any event that isn't from the Events API (makes sense, app.event() is only for these kinds of events). matchEventType('app_home_opened') will filter out any event that doesn't have a type matching app_home_opened. likewise matchEventType('app_mentioned') will filter out any event that doesn't have a type matching app_mentioned.

but wait this means in order for the listener to be called, the incoming event mush both match app_home_opened and app_mentioned. earlier, my guess was that it was either, however now we know that's not the case. in fact, can any event type actually match several names? is that a useful thing to implement?

hint: regular expressions are great at expressing how patterns should compose to form a match. what do i mean by "how patterns should compose?". specifically, the idea of AND versus OR between patterns can be expressed through a regular expression. /app_home_opened|app_mentioned/ is a perfectly reasonable way to express that the type should match either those two types.

Thanks for the code review and comments! It was very helpful in understanding the issue. There should be only one event name or pattern preceding the listener function. I have fixed the issue and committed the changes.

aoberoi · 2021-01-25T20:57:40Z

src/App.ts

  ): void {
-    this.listeners.push([onlyEvents, matchEventType(eventName), ...listeners] as Middleware<AnyMiddlewareArgs>[]);
+    const eventMiddleware = patternsOrMiddleware.map((patternOrMiddleware) => {


what does this code imply about the order of patterns versus the listener? specifically, should the following code be allowed?

app.event('app_home_opened', async () => { /* ... some listener code ... */}, 'foo_bar_baz')

With the updated code changes, arguments should be passed in an order(event name pattern, listener).

aoberoi · 2021-01-25T20:59:17Z

src/middleware/builtin.spec.ts

+    return {
+      payload: appHomeOpenedEvent,
+      event: appHomeOpenedEvent,
+      message: null as never, // a bit hackey to sartisfy TS compiler


good job including a comment to justify the use of the type assertion! i wish i knew a little more about what the TS compiler's complaint would have been if you hadn't used the type assertion.

I have updated the comments with the reason for using type assertion here.

aoberoi · 2021-01-25T20:59:48Z

src/middleware/builtin.spec.ts

@@ -609,6 +630,30 @@ describe('matchEventType', () => {
    assert.isTrue(fakeNext.called);
  });

+  it('should detect valid RegExp requests with app_mention', async () => {
+    const fakeNext = sinon.fake();
+    await matchEventType(/app_mention|app_home_opened/)({


perfect! this is the intended usage and i'm glad you specifically have a test that illustrates it.

aoberoi · 2021-01-25T21:00:22Z

src/middleware/builtin.spec.ts

+
+  it('should detect valid RegExp requests with app_home_opened', async () => {
+    const fakeNext = sinon.fake();
+    await matchEventType(/app_mention|app_home_opened/)({


what's the difference between this test case and the one above it?

There's no big difference, I just wanted to cover tests for the regex(/app_mention|app_home_opened/) specific to app_home_opened. We can remove this test case if it’s unnecessary to have redundant tests.
.

aoberoi · 2021-01-25T21:00:56Z

src/middleware/builtin.spec.ts

+
+  it('should skip other requests for RegExp', async () => {
+    const fakeNext = sinon.fake();
+    await matchEventType(/foo/)({


good call to have a test case for both matching and not matching.

seratch · 2021-02-02T08:53:46Z

I just reran the GitHub Actions job. Now the all the checks pass.

seratch

LGTM

seratch · 2021-02-02T09:49:20Z

Although I already gave an approval to this pull request, let me share a minor concern in regard to a future enhancement for TypeScript users.

As of today, app.event does not automatically determine the type for an event payload (if I understand correctly). If we aspire to provide more type-safety for event/payload arg for listener functions in the future, having this regular expression can be an obstacle (or it may make the implementation a bit complex). We may be able to come up with union types for it. But obviously it's not deterministic from a given regular expression. For supporting this TS use case, we may need to have type argument like app.event<'app_home_opened' | 'message'>(/app_home_opened|message/, async ({}) => {})

That being said, this addition should be greatly helpful in many situations. Therefore, I'm positive to have this. I just wanted to mention this limitation for future maintainers and/or contributors.

gitwave bot added the untriaged label Jan 21, 2021

mwbrooks added enhancement M-T: A feature request for new functionality semver:minor and removed untriaged labels Jan 21, 2021

mwbrooks requested review from stevengill, seratch, mwbrooks, misscoded and aoberoi January 21, 2021 00:53

Fixed slackapi#284 -added support for regexp to events

b0378dd

pdontha force-pushed the issue-284-regex-events branch from 1c9234c to b0378dd Compare January 21, 2021 00:59

prettier style changes

5a07521

Merge branch 'main' into issue-284-regex-events

7e59d38

aoberoi suggested changes Jan 25, 2021

View reviewed changes

pdontha and others added 2 commits January 30, 2021 22:09

Merge branch 'main' into issue-284-regex-events

cc5ec8d

Fix to handle only single eventname or pattern

b5f62d2

seratch approved these changes Feb 2, 2021

View reviewed changes

pdontha added 2 commits February 3, 2021 13:59

Merge branch 'main' into issue-284-regex-events

9e441a5

Merge branch 'main' into issue-284-regex-events

859d2d4

stevengill approved these changes Feb 9, 2021

View reviewed changes

stevengill and others added 3 commits February 9, 2021 15:33

Merge branch 'main' into issue-284-regex-events

3b1a677

Merge branch 'main' into issue-284-regex-events

6cce7cd

Merge branch 'main' into issue-284-regex-events

0c81c26

misscoded merged commit 0eca1ba into slackapi:main Feb 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 284 regex events #763

Issue 284 regex events #763

pdontha commented Jan 21, 2021

CLAassistant commented Jan 21, 2021 •

edited

Loading

mwbrooks commented Jan 21, 2021

codecov bot commented Jan 21, 2021 •

edited

Loading

pdontha commented Jan 21, 2021

aoberoi Jan 25, 2021

pdontha Feb 2, 2021

aoberoi Jan 25, 2021

pdontha Feb 2, 2021

aoberoi Jan 25, 2021

pdontha Feb 2, 2021

aoberoi Jan 25, 2021

aoberoi Jan 25, 2021

pdontha Feb 2, 2021

aoberoi Jan 25, 2021

seratch commented Feb 2, 2021

seratch left a comment

seratch commented Feb 2, 2021

Issue 284 regex events #763

Issue 284 regex events #763

Conversation

pdontha commented Jan 21, 2021

Summary

Requirements (place an x in each [ ])

CLAassistant commented Jan 21, 2021 • edited Loading

mwbrooks commented Jan 21, 2021

codecov bot commented Jan 21, 2021 • edited Loading

Codecov Report

pdontha commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Feb 2, 2021

seratch left a comment

Choose a reason for hiding this comment

seratch commented Feb 2, 2021

Requirements (place an `x` in each `[ ]`)

CLAassistant commented Jan 21, 2021 •

edited

Loading

codecov bot commented Jan 21, 2021 •

edited

Loading