fix(cli-hooks): silence node warnings that can break @slack/cli-hooks

mwbrooks commented 2 days ago

Summary

This pull request updates the @slack/cli-hooks to silence the node process warning output.

The reason for this update is that warnings can break some hooks, such as get-manifest, because the output must be valid JSON and the warning message is plain-text. For other hooks, such as doctor the warning is displayed to the user when the CLI command is executed, which can be confusing. I've left the warning for start because the user's code may need to print warning messages to the console.

Example of a node warning message

This is an example using node v23.2.0:

$ npx slack-cli-get-hooks

(node:52368) ExperimentalWarning: CommonJS module /Users/michael.brooks/.nvm/versions/node/v23.2.0/lib/node_modules/npm/node_modules/debug/src/node.js is loading ES Module /Users/michael.brooks/.nvm/versions/node/v23.2.0/lib/node_modules/npm/node_modules/supports-color/index.js using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
{"hooks":{"doctor":"NODE_NO_WARNINGS=1 npx -q --no-install -p @slack/cli-hooks slack-cli-doctor","check-update":"NODE_NO_WARNINGS=1 npx -q --no-install -p @slack/cli-hooks slack-cli-check-update","get-manifest":"NODE_NO_WARNINGS=1 npx -q --no-install -p @slack/cli-hooks slack-cli-get-manifest","start":"npx -q --no-install -p @slack/cli-hooks slack-cli-start"},"config":{"watch":{"filter-regex":"^manifest\\.json$","paths":["."]},"protocol-version":["message-boundaries"],"sdk-managed-connection-enabled":true},"runtime":"node"}

Expected `get-hooks` JSON

{
   "config" : {
      "protocol-version" : [
         "message-boundaries"
      ],
      "sdk-managed-connection-enabled" : true,
      "watch" : {
         "filter-regex" : "^manifest\\.json$",
         "paths" : [
            "."
         ]
      }
   },
   "hooks" : {
      "check-update" : "NODE_NO_WARNINGS=1 npx -q --no-install -p @slack/cli-hooks slack-cli-check-update",
      "doctor" : "NODE_NO_WARNINGS=1 npx -q --no-install -p @slack/cli-hooks slack-cli-doctor",
      "get-manifest" : "NODE_NO_WARNINGS=1 npx -q --no-install -p @slack/cli-hooks slack-cli-get-manifest",
      "start" : "npx -q --no-install -p @slack/cli-hooks slack-cli-start"
   },
   "runtime" : "node"
}

Reviewers

# Change into the cli-hooks package
$ cd node-slack-sdk/packages/cli-hooks

# Install the dependencies
$ npm install

# Run the get-hooks and (optionally) format the JSON
$ npx slack-cli-get-hooks | json_pp

# Expect the JSON response above
# - Confirm `NODE_NO_WARNINGS=1` for `check-update`, `doctor`, and `get-manifest`
# - You may see a warning when you run `get-hooks`, this warning can be safely ignored
#   because it will be silenced in `slack.json`

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

[x] I've read and understood the Contributing Guidelines and have done my best effort to follow them.
[x] I've read and agree to the Code of Conduct.

codecov[bot] commented 2 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.66%. Comparing base (278514f) to head (a24f387). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2096 +/- ## ======================================= Coverage 91.65% 91.66% ======================================= Files 38 38 Lines 10311 10313 +2 Branches 647 647 ======================================= + Hits 9451 9453 +2 Misses 848 848 Partials 12 12 ``` | [Flag](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2096/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | Coverage Δ | | |---|---|---| | [cli-hooks](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `95.24% <100.00%> (+0.01%)` | :arrow_up: | | [cli-test](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `94.47% <ø> (ø)` | | | [oauth](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `77.39% <ø> (ø)` | | | [socket-mode](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `58.22% <ø> (ø)` | | | [web-api](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `96.88% <ø> (ø)` | | | [webhook](https://app.codecov.io/gh/slackapi/node-slack-sdk/pull/2096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi) | `96.65% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=slackapi#carryforward-flags-in-the-pull-request-comment) to find out more.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

mwbrooks commented 14 hours ago

We can merge this whenever, but I'm hoping we can instead separate stdout and stderr as part of the default hook protocol on the CLI side of things 👀 IMO debug printing possible warnings would be more ideal for finding odd configurations or other errors that might cause later problems with hooks.

I'm also very much in favour of updating the Slack CLI to separate stdout and stderr. We may also want the combined output and possibly the error code.

It would be nice to have a future where all stdout and stderr are printed to --verbose while select output is displayed in the standard output. This would allow developers to debug while keeping the normal CLI output cleaner.

I also share the concern that meaningful warnings could be suppressed. For example, I think NODE_NO_WARNINGS=1 will also silence console.warn(...) which could be used by developers and the Bolt Framework for debugging.

That shouldn't block this, but I'm thinking we can revert these changes if that lands while these warnings are only appearing in unsupported Node versions?

I'm in favour of reverting this once we improve the CLI as well!

I'm also wondering if we improve testing in later PRs to exercise the hooks in multiple versions of multiple runtimes 😉

We should. I imagine the hooks package should support the same node versions that the Slack SDK and Bolt Frameworks support?

zimeg commented 12 hours ago

It would be nice to have a future where all stdout and stderr are printed to --verbose while select output is displayed in the standard output. This would allow developers to debug while keeping the normal CLI output cleaner.

Wow! Mirroring stdout in debugs is a fantastic idea! Filtering with the tagged INFO or ERROR will be so helpful 😭

I also share the concern that meaningful warnings could be suppressed.

I'm hoping to start a PR that separates these outputs soon! I hadn't thought about the console.warn suppressions, but these are the edges I fear so much 😉

I imagine the hooks package should support the same node versions that the Slack SDK and Bolt Frameworks support

Right now I think we're testing these versions alright, but I don't think we test that an actual npx slack-cli-get-hooks call returns just the JSON. That might be difficult to test for some hooks that depend on app setups - start - but a single test for get-hooks might catch regressions from warnings like this!

This might also not be a problem after hook enhancements!

I left another comment about testing (not necessarily supporting) the current Node version to make upgrades to the next LTS a bit easier, but that might be discussion for future changes! 🤔

This does seem good to merge as is though! Please of course feel free to tag a release too if you'd like. Or let me know if that's a tag I can make. I still plan to follow up with reversions and revisions in the CLI, but let's not block improvement here 💪

slackapi / node-slack-sdk