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

[mwbrooks]

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 [ ])

• 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.

[mwbrooks]

Linter auto-pilot

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           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	Coverage Δ
cli-hooks	95.24% <100.00%> (+0.01%)	⬆️
cli-test	94.47% <ø> (ø)
oauth	77.39% <ø> (ø)
socket-mode	58.22% <ø> (ø)
web-api	96.88% <ø> (ø)
webhook	96.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here 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

[zimeg]

LGTM and thanks for updating the README too! 📚

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.

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 also wondering if we improve testing in later PRs to exercise the hooks in multiple versions of multiple runtimes 😉

[zimeg]

🙏 Nice find! I hadn't noticed these warnings in the versions we officially support - 18, 20, 22 for now - but think this is a nice and quick guard for those on the cutting edge.

[mwbrooks]

Thanks! I'm honestly a little confused around why the warnings are happening only 23. A long time ago, node used odd majors as "experimental" releases and even majors as "stable" releases, but I thought they stopped that pattern.

[zimeg]

The Node release schedule is so interesting to me!

I believe odd versions are treated as a "current" release and act as an upgrading version to the next even version. These odd versions never get long term support 😅

We might want to discuss testing with the latest current version across the SDKs to make sure bumping to the next current LTS goes smooth.

Supporting that version IMO is another question 🤔

[mwbrooks]

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]

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 💪