Introduce a `@slack/cli-hooks` package that implements Slack CLI hooks #1714

zimeg · 2024-01-08T22:07:00Z

Summary

This PR introduces a standalone package that implements hooks for the Slack CLI, meant for use in Bolt for JavaScript apps.

Early versions of this package are published to NPM here: https://www.npmjs.com/package/@slack/cli-hooks

Reviewers

Update a new Bolt app with the latest version of the CLI and the following commands:

$ slack create hooks --template slack-samples/bolt-js-custom-function-template

# Confirm a manifest.json file is found
$ slack manifest

# Check that the app starts without problems
$ slack run

# Verify dependencies with updates are shown
# Note: Installing updates is not yet supported
$ slack update

Notes

A few quick questions rose during development:

~~Should the start hook default file be app.js or index.js?~~ 👍
~~Is the install-update hook alright to implement as a follow up?~~ 👍

~~Some tasks remain as TODOs as well: #1714 (comment)~~ 👍

Requirements

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.

- feat: introduce shared linter configurations - fix: fix common linting errors - refactor: update checking for the check update hook - build: add a build step to transpile typescript

packages/hooks/README.md

packages/hooks/package.json

packages/hooks/src/check-update.js

misscoded · 2024-01-11T03:12:18Z

packages/hooks/src/check-update.js

+    return version;
+  });
+  return {
+    name: 'the Slack SDK',


Is this correct?

Not sure how correct it is, but it matches the Deno hooks implementation and makes some sense to me. This field "corresponds to the overall package/library, in which individual component releases are bundled" and I'm not sure of better naming for it since both @slack/bolt and @slack/hooks might be included. Open to any suggestions though!

I've always disliked what we chose for the Deno one since it feels so general. No harm though in staying the course: let's keep it for now!

misscoded · 2024-01-11T03:13:02Z

packages/hooks/src/check-update.js

+    name: 'the Slack SDK',
+    message: '',
+    releases,
+    url: 'https://api.slack.com/automation/changelog',


Again, is this what we would want to point them to?

🤔 Not sure of a better place at the moment... It does seem strange since CLI release notes are displayed separately from this and the automation changelog is mostly CLI and Deno. I'm open to changing this whenever!

For reference here are example update logs

$ slack update 🌱 A new version of the Slack CLI is available: v1.0.0 → 2.15.0 You can read the release notes at: https://api.slack.com/automation/changelog To manually update, visit the download page: https://api.slack.com/automation/cli/install ? 🚀 Do you want to auto-update to the latest version now? No 🛠️ An update from the Slack SDK is available: › @slack/hooks 0.0.5 → 0.0.6 Learn more at: https://github.com/slackapi/node-slack-sdk/releases/tag/@slack/[email protected] For more information about this update, visit: https://api.slack.com/automation/changelog

Ah, thanks for the visual! That's helpful. I guess my question is will the hooks package update be present in the Changelog with everything else? If so, it makes sense. If not, we probably need to tweak the CLI logic at some point.

+1 to future changes in the CLI since I don't think these hooks will be included in the changelog / follow the same release cycle. Maybe we do align these releases though?

For now I think this is alright to leave but will add it to the backlog!

packages/hooks/src/get-hooks.js

packages/hooks/src/start.js

Co-authored-by: Alissa Renz <[email protected]>

zimeg

@misscoded Really appreciate the review! Going to follow up with a few updates to the README and an update to get-manifest but will hold off on changes to naming and removal of Deno for right now. Definitely open to making these changes soon though!

packages/hooks/README.md

packages/hooks/src/check-update.js

zimeg · 2024-01-11T05:33:55Z

packages/hooks/src/check-update.js

+    return version;
+  });
+  return {
+    name: 'the Slack SDK',


Not sure how correct it is, but it matches the Deno hooks implementation and makes some sense to me. This field "corresponds to the overall package/library, in which individual component releases are bundled" and I'm not sure of better naming for it since both @slack/bolt and @slack/hooks might be included. Open to any suggestions though!

zimeg · 2024-01-11T05:39:46Z

packages/hooks/src/check-update.js

+    name: 'the Slack SDK',
+    message: '',
+    releases,
+    url: 'https://api.slack.com/automation/changelog',


🤔 Not sure of a better place at the moment... It does seem strange since CLI release notes are displayed separately from this and the automation changelog is mostly CLI and Deno. I'm open to changing this whenever!

For reference here are example update logs

$ slack update 🌱 A new version of the Slack CLI is available: v1.0.0 → 2.15.0 You can read the release notes at: https://api.slack.com/automation/changelog To manually update, visit the download page: https://api.slack.com/automation/cli/install ? 🚀 Do you want to auto-update to the latest version now? No 🛠️ An update from the Slack SDK is available: › @slack/hooks 0.0.5 → 0.0.6 Learn more at: https://github.com/slackapi/node-slack-sdk/releases/tag/@slack/[email protected] For more information about this update, visit: https://api.slack.com/automation/changelog

packages/hooks/src/start.js

packages/hooks/src/get-hooks.js

packages/hooks/README.md

WilliamBergamin

This looks great 💯 I left a few suggestions and NITs

Should the start hook default file be app.js or index.js?

Our templates and samples use app.js defaulting to it makes sense

Is the install-update hook alright to implement as a follow up?

I say go for it but I think there is also value in implementing the message boundary protocol

packages/hooks/src/check-update.js

packages/hooks/src/get-hooks.js

filmaj · 2024-01-11T18:46:26Z

Gonna start my review now, but re: your questions in your original comment:

Should the start hook default file be app.js or index.js?

slack.dev getting started instructions use app.js, so that has my vote.

Is the install-update hook alright to implement as a follow up?

100%

filmaj · 2024-01-11T19:23:12Z

@zimeg one thing I noticed: I have the sample up and running, but I can't add the function bundled in the app to WFB when I deploy it to my test workspace. I think because the output of slack manifest doesn't include any functions, so there's no 'custom step' in WFB to add.

zimeg · 2024-01-26T03:17:20Z

@misscoded @WilliamBergamin @filmaj A few patches later and I believe this is ready for re-review! I've updated the steps to review to hopefully be easier (everything should be working right away) but you will need a dev build of the CLI to handle automagic tokens.

Please let me know if there's anything more that needs updating! Or just changing! 🙏

filmaj · 2024-01-26T15:47:21Z

Two paper-cut style issues I just encountered (feel free to file somewhere and move on if you think this is something worth addressing at some point):

`create --template` clones using https://

I had some issues testing this out from scratch, I think because the template repo you linked to is internal:

10:40:14 in ~/src
➜ slak create remote-func-js --template slack-samples/bolt-js-custom-function-template
⚙️  Creating a new Slack app in ~/src/remote-func-js

Username for 'https://github.com': filmaj
Password for 'https://[email protected]':
Check /Users/fmaj/.slack/logs/slack-debug-20240126.log for full error logs

🚫 Error: While cloning repository, the following error occurred:

git clone --depth=1 https://github.com/slack-samples/bolt-js-custom-function-template.git /Users/fmaj/src/remote-func-js

Cloning into '/Users/fmaj/src/remote-func-js'...
remote: Support for password authentication was removed on August 13, 2021.

Even entering my username and password doesn't work as GitHub seems to block those attempts.

Idea: can the CLI fallback to trying to clone using SSH if cloning using HTTPS fails?

Error returned if bolt-js dependencies not installed is unclear

Another sharp edge: I then tried just manually cloning the repo and running slack manifest within - but forgetting to run npm i within. The error I get is:

➜ slak manifest
Check /Users/fmaj/.slack/logs/slack-debug-20240126.log for full error logs

🚫 There was an error while reading the Slack Configuration file (`slack.json`) or running the `get-hooks` hook (sdk_config_load_error)

Error Details:

1: Command for '' returned an error: exit status 1
npm ERR! canceled

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/fmaj/.npm/_logs/2024-01-26T15_44_44_782Z-debug-0.log (sdk_hook_invocation_failed)
Suggestion: Run `slack doctor` to check that your system dependencies are up-to-date.


Suggestion:

Run `slack doctor` to check that your system dependencies are up-to-date.

Is there any way to return a nicer / clearer error in this case?

filmaj · 2024-01-26T15:56:01Z

But - in general works GREAT! AWESOME!

filmaj

Awesome work, very close to being ready I think!

packages/cli-hooks/src/check-update.js

filmaj · 2024-01-26T16:03:39Z

packages/cli-hooks/src/get-hooks.js

+
+/**
+ * Standardized communication format between the SDK and CLI regarding hooks.
+ * @typedef SDKInterface


Great job documenting these

filmaj · 2024-01-26T16:04:50Z

packages/cli-hooks/src/get-hooks.js

+      'sdk-managed-connection-enabled': true,
+    },
+    runtime: 'node',
+  };


The cleanliness of this implementation gives me the warm n fuzzies

packages/cli-hooks/src/get-manifest.js

packages/cli-hooks/package.json

packages/cli-hooks/src/protocols.js

filmaj · 2024-01-26T16:21:25Z

packages/cli-hooks/src/get-hooks.js

+          '.',
+        ],
+      },
+      'protocol-version': ['message-boundaries', 'default'],


Should we pull these protocol names from the protocol source file in this package? The reason I bring this up is that protocol.js defines a SUPPORTED_NAMED_PROTOCOLS array that only includes the message boundary protocol, but by returning this line in get-hooks, we're telling the CLI that either of the two named protocols is fine to use (with a preference for message-boundaries). We should decide on one approach.

Totally in favor of using SUPPORTED_NAMED_PROTOCOLS - good call. Making the update now to use that exported array with only message-boundaries for now. Open to also including default in the export, but I lean towards including just the message-boundaries in this hook.

packages/cli-hooks/src/protocols.spec.js

filmaj · 2024-01-26T16:25:22Z

packages/cli-hooks/src/protocols.spec.js

+    });
+  });
+
+  describe('message boundary protocol', () => {


Excellent work!

packages/cli-hooks/src/start.js

WilliamBergamin

This is awesome 💯 just left some nits take them or leave them

packages/cli-hooks/src/protocols.js

packages/cli-hooks/src/check-update.js

packages/cli-hooks/src/protocols.js

zimeg · 2024-01-26T20:02:45Z

@filmaj @WilliamBergamin appreciate another round of reviews and pointers on polish! I think I've addressed all of the comments and suggestions (or added more polish-related CLI-things to our backlog) so I think this is ready for another round of review.

I've tested this on Node v18.19.0 and v20.8.0 for macOS and Node v18.19.0 and v20.11.0 for Windows. Everything is working as hoped! There is a problem forming the actual web socket connection on Windows, but I think this is a problem with the virtual environment I'm using and not the hooks (the app is stuck trying to authenticate the Socket Mode request) 😬

filmaj

zimeg · 2024-01-26T21:24:21Z

@filmaj :excited: 🚀 let's goooooo!! Do we want to start this at v1.0.0 or stay in the patch range for now? Or maybe that's extreme and we want something more minor?

filmaj · 2024-01-26T21:29:31Z

Your call big guy!

zimeg · 2024-01-26T21:44:02Z

@filmaj The time feels right for a 1.0.0 😎

zimeg · 2024-01-26T22:02:02Z

@filmaj @WilliamBergamin @misscoded Really appreciate all of the reviews on this! Thanks a ton! 🚢 💨

feat: introduce a package for slack cli hooks

a6c1ffb

zimeg added semver:minor enhancement M-T: A feature request for new functionality pkg:cli-hooks applies to `@slack/cli-hooks` labels Jan 8, 2024

zimeg self-assigned this Jan 8, 2024

zimeg added 12 commits January 8, 2024 17:22

fix: correct errors in hooks package before publishing

791d35e

feat: convert scripts to typescript for type safety

49e6f3d

- feat: introduce shared linter configurations - fix: fix common linting errors - refactor: update checking for the check update hook - build: add a build step to transpile typescript

chore: bump package version after accidental publish

808fba4

feat: remove a build step but keep typings with jsdoc

8a889eb

chore: merge w main

5827a8c

test: add coverage and fix problems in the scripts

e169b70

ci: include the hooks package in automated tests

63ba992

ci: continue running all tests even if one fails

bd4da54

test: compare error outputs of different node versions

cbdfb7d

docs: include instructions for app setup in the readme

938dc24

chore: merge w main

fb2ee36

chore: bump the package version another patch

0fbb96e

misscoded reviewed Jan 11, 2024

View reviewed changes

misscoded requested review from filmaj and WilliamBergamin January 11, 2024 03:22

style: improve wording and such semantics

54d05a9

Co-authored-by: Alissa Renz <[email protected]>

zimeg commented Jan 11, 2024

View reviewed changes

zimeg added 3 commits January 10, 2024 22:13

test: update expected output with actual output

81e7a6a

fix: remove script extensions from get-manifest

75d1994

docs: switch information about setup for hooks details

f04d115

zimeg commented Jan 11, 2024

View reviewed changes

packages/hooks/README.md Outdated Show resolved Hide resolved

zimeg marked this pull request as ready for review January 11, 2024 07:49

WilliamBergamin requested changes Jan 11, 2024

View reviewed changes

packages/hooks/src/check-update.js Outdated Show resolved Hide resolved

packages/hooks/src/check-update.js Outdated Show resolved Hide resolved

packages/hooks/src/get-hooks.js Outdated Show resolved Hide resolved

zimeg added 2 commits January 25, 2024 17:15

feat: remove extraneous logs from the start hook

cb1c85e

chore: bump the package patch version

614a965

zimeg requested review from misscoded, WilliamBergamin and filmaj January 26, 2024 03:17

filmaj requested changes Jan 26, 2024

View reviewed changes

WilliamBergamin reviewed Jan 26, 2024

View reviewed changes

packages/cli-hooks/src/protocols.js Outdated Show resolved Hide resolved

packages/cli-hooks/src/check-update.js Show resolved Hide resolved

packages/cli-hooks/src/protocols.js Outdated Show resolved Hide resolved

zimeg added 7 commits January 26, 2024 10:09

test: display coverage after testing results

f923d94

fix: use the exported SUPPORTED_NAMED_PROTOCOLS in get-hooks

91e139a

test: assert missing boundary error text matches

9a2b931

docs: improve remediation errors for missing start tokens

c7ded6e

refactor: call the base protocol the default protocol

6fe204b

refactor: get the protocol implementation and not an interface

c3b804e

chore: bump the package patch version

980eee0

filmaj approved these changes Jan 26, 2024

View reviewed changes

chore: release a major version of this package

ab411ac

zimeg added semver:major release and removed semver:minor labels Jan 26, 2024

zimeg added this to the [email protected] milestone Jan 26, 2024

zimeg merged commit f3dff4d into main Jan 26, 2024
17 checks passed

zimeg deleted the cli-hooks branch January 26, 2024 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a `@slack/cli-hooks` package that implements Slack CLI hooks #1714

Introduce a `@slack/cli-hooks` package that implements Slack CLI hooks #1714

zimeg commented Jan 8, 2024 •

edited

Loading

misscoded Jan 11, 2024

zimeg Jan 11, 2024

misscoded Jan 11, 2024

misscoded Jan 11, 2024

zimeg Jan 11, 2024

misscoded Jan 11, 2024

zimeg Jan 12, 2024

zimeg left a comment

zimeg Jan 11, 2024

zimeg Jan 11, 2024

WilliamBergamin left a comment

filmaj commented Jan 11, 2024

filmaj commented Jan 11, 2024

zimeg commented Jan 26, 2024

filmaj commented Jan 26, 2024 •

edited

Loading

filmaj commented Jan 26, 2024

filmaj left a comment

filmaj Jan 26, 2024

filmaj Jan 26, 2024

filmaj Jan 26, 2024

zimeg Jan 26, 2024

filmaj Jan 26, 2024

WilliamBergamin left a comment

zimeg commented Jan 26, 2024 •

edited

Loading

filmaj left a comment

zimeg commented Jan 26, 2024

filmaj commented Jan 26, 2024

zimeg commented Jan 26, 2024

zimeg commented Jan 26, 2024

Introduce a @slack/cli-hooks package that implements Slack CLI hooks #1714

Introduce a @slack/cli-hooks package that implements Slack CLI hooks #1714

Conversation

zimeg commented Jan 8, 2024 • edited Loading

Summary

Reviewers

Notes

Requirements

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

zimeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBergamin left a comment

Choose a reason for hiding this comment

filmaj commented Jan 11, 2024

filmaj commented Jan 11, 2024

zimeg commented Jan 26, 2024

filmaj commented Jan 26, 2024 • edited Loading

create --template clones using https://

Error returned if bolt-js dependencies not installed is unclear

filmaj commented Jan 26, 2024

filmaj left a 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

WilliamBergamin left a comment

Choose a reason for hiding this comment

zimeg commented Jan 26, 2024 • edited Loading

filmaj left a comment

Choose a reason for hiding this comment

zimeg commented Jan 26, 2024

filmaj commented Jan 26, 2024

zimeg commented Jan 26, 2024

zimeg commented Jan 26, 2024

Introduce a `@slack/cli-hooks` package that implements Slack CLI hooks #1714

Introduce a `@slack/cli-hooks` package that implements Slack CLI hooks #1714

zimeg commented Jan 8, 2024 •

edited

Loading

filmaj commented Jan 26, 2024 •

edited

Loading

`create --template` clones using https://

zimeg commented Jan 26, 2024 •

edited

Loading