Skip to content

Conversation

@ledmonster
Copy link

@ledmonster ledmonster commented Sep 7, 2025

Summary

Requirements (place an x in each [ ])

Note

  • please let me know how I can test the behavior.

@salesforce-cla
Copy link

salesforce-cla bot commented Sep 7, 2025

Thanks for the contribution! Before we can merge this, we need @ledmonster to sign the Salesforce Inc. Contributor License Agreement.

@ledmonster ledmonster marked this pull request as ready for review September 7, 2025 12:23
@ledmonster ledmonster requested a review from a team as a code owner September 7, 2025 12:23
@zimeg zimeg changed the title use deno-slack-hooks instead of deno-slack-builder fix: gather app manifest using deno-slack-hooks instead of deno-slack-builder Sep 8, 2025
@zimeg zimeg added bug Something isn't working semver:patch requires a patch version number bump dependencies Pull requests that update a dependency file labels Sep 8, 2025
@codecov
Copy link

codecov bot commented Sep 8, 2025

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.41%. Comparing base (5195ed2) to head (f087c30).

Files with missing lines Patch % Lines
src/local-run-function.ts 75.00% 1 Missing ⚠️
src/local-run.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   93.22%   93.41%   +0.18%     
==========================================
  Files          17       17              
  Lines         620      607      -13     
  Branches       93       93              
==========================================
- Hits          578      567      -11     
+ Misses         42       40       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ledmonster LGTM! And thank you for taking the time to send this fix in 📬 ✨

To test this I updated a hooks.json file with the following to match the setups of slackapi/deno-slack-sdk#375 (comment) with this start hook:

{
  "hooks": {
    "start": "deno run -q --config=deno.jsonc --allow-read --allow-net --allow-run --allow-env --allow-import --allow-sys=osRelease https://raw.githubusercontent.com/slackapi/deno-slack-runtime/9c85a5adcd285aba0d6f2d280894755274055f4e/src/local-run.ts "
  }
}

Approving this now with a fix to no longer use the "builder" package, but I believe some follow up elsewhere is still needed to avoid import errors. I'm still noticing multiple requests for the same "import" permission when using this command and it's not clear to me why the "--allow-import" flag isn't being used:

$ DENO_TRACE_PERMISSIONS=1 deno run -q --config=deno.jsonc --allow-read --allow-net --allow-run --allow-env --allow-import --allow-sys=osRelease https://raw.githubusercontent.com/slackapi/deno-slack-runtime/9c85a5adcd285aba0d6f2d280894755274055f4e/src/local-run.ts

┏ ⚠️  Deno requests import access to "googleapis.deno.dev:443".
┠─ Requested by `import()` API.
┃  ├─ op_fs_stat_async (ext:core/00_infra.js:249:44)
┃  ├─ Object.stat (ext:deno_fs/30_fs.js:410:21)
┃  ├─ readImportedManifestFile (https://deno.land/x/[email protected]/get_manifest.ts:107:35)
┃  ├─ getManifest (https://deno.land/x/[email protected]/get_manifest.ts:33:28)
┃  ├─ eventLoopTick (ext:core/01_core.js:179:7)
┃  ├─ async runLocally (https://raw.githubusercontent.com/slackapi/deno-slack-runtime/9c85a5adcd285aba0d6f2d280894755274055f4e/src/local-run-function.ts:21:20)
┃  └─ async https://raw.githubusercontent.com/slackapi/deno-slack-runtime/9c85a5adcd285aba0d6f2d280894755274055f4e/src/local-run-function.ts:51:3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cla:signed dependencies Pull requests that update a dependency file semver:patch requires a patch version number bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants