improved type resolution for action method

[stevengill]

Summary

Related to #326 and #325.

A small improvement based on comments by @aoberoi at #326 (comment).

Now the action argument gets narrowed down to appropriate action based on type. Example:

app1.action({ type: 'block_actions' }, ({ action }) => {
// action is now BlockAction instead of SlackAction
});

Currently, no support for the other two use cases @aoberoi pointed out:

1. Typescript should complain about a type:message_action also passing an action_id (action_id is specific to BlockActions)
2. Helping a developer understand what the options might be for the type constraint by allowing your editor's inference to give you suggestions.

Still need to add tests for these changes.

Let me know your thoughts!

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.

[codecov]

Codecov Report

Merging #349 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #349   +/-   ##
=====================================
  Coverage      73%    73%           
=====================================
  Files           7      7           
  Lines         500    500           
  Branches      146    146           
=====================================
  Hits          365    365           
  Misses        103    103           
  Partials       32     32

Impacted Files	Coverage Δ
src/App.ts	78.23% <ø> (ø)	⬆️

---

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 d59319c...8bb6838. Read the comment docs.

[seratch]

I verified how it works on Visual Studio Code and understood this is greatly beneficial. I like this idea very much 👍

block_actions

message_action

[stevengill]

Thanks @seratch for the great review and including screenshots!

I added dtslint tests to this PR with 5ae4ba5.

I followed the pattern from node-slack-sdk to make the integration tests their own package unaware of local files to simulate a real world experience.

Integration tests will test the latest released version of bolt. The issue here is that the released version of bolt doesn't include the changes from this PR to test. So I ended up doing a npm link in my local @slack/bolt repo and then ran npm link @slack/bolt in my integration tests folder. This allowed me to run these tests against my local bolt.

I'll leave a few comments in line myself that hopefully @aoberoi can answer.

[stevengill]

One other thing I forgot to mention, I haven't tied this into the global npm test yet (so no travisCI integration). Definitely will want to do that, but maybe after the next bolt release so our CI doesn't go red.

[aoberoi]

Integration tests will test the latest released version of bolt. The issue here is that the released version of bolt doesn't include the changes from this PR to test. So I ended up doing a npm link in my local @slack/bolt repo and then ran npm link @slack/bolt in my integration tests folder. This allowed me to run these tests against my local bolt.

@stevengill is this something we can automate in the scripts that run on CI? it drastically reduces the usefulness of these tests in CI if they test against the latest release as opposed to the changes being made in the branch/PR.

Maybe we can use a local pathin the package.json entry for @slack/bolt instead of a version number? i haven't tried this in a while, so i'm not sure if there are any downsides to it.

[aoberoi]

A couple small nits, but overall looks great!

[aoberoi]

Once this PR lands, can we open an issue to investigate how we might get this test (and the last one in this file) to pass? I have an idea that might work, but its not worth blocking this PR over.

[aoberoi]

btw, i tried out the idea for a couple mins and couldn't get it to work. doesn't mean its not possible, but would likely take more time than i have today to fully investigate.

[aoberoi]

should this be the npm test script instead of directly trying to run dtslint?

[stevengill]

I tried that by following https://code.visualstudio.com/docs/nodejs/nodejs-debugging, but it seems very specific to the debug command. I've tried swapping it for npm test, but it just hangs.

        {
            "type": "node",
            "request": "launch",
            "name": "npm test",
            "cwd": "${workspaceFolder}",
            "runtimeExecutable": "npm",
            "runtimeArgs": [
                "run-script", "test"
            ],
            "port": 9229
        },

[stevengill]

I've pushed changes based on the feedback.

I've updated the bolt dependency to use the local bolt instead of the released one. Seems to be working fine.

I'll do one more update so the global npm test will run these integration tests too.