Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update esbuild dependencies for bundling #94

Merged
merged 10 commits into from
Aug 29, 2024
Merged

Conversation

WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented Aug 28, 2024

Summary

This PR update the esbuild related dependency and improves support for bundling npm packages.

It also gets ride of the deps.ts file. This prevents every hook from importing all the project dependencies when executing.

Testing

  1. Create a new app
  2. Use the following slack.json
{
  "hooks": {
    "get-hooks": "deno run -q --allow-read --allow-net path/to/deno-slack-hooks/src/mod.ts",
    "get-manifest": "deno run -q --config=deno.jsonc --allow-read --allow-net --allow-env --allow-sys=osRelease path/to/deno-slack-hooks/src/get_manifest.ts",
    "build": "deno run -q --config=deno.jsonc --allow-read --allow-write --allow-net --allow-run --allow-env --allow-sys=osRelease path/to/deno-slack-hooks/src/build.ts",
    "get-trigger": "deno run -q --config=deno.jsonc --allow-read --allow-net --allow-env path/to/deno-slack-hooks/src/get_trigger.ts",
    "check-update": "deno run -q --config=deno.jsonc --allow-read --allow-net path/to/deno-slack-hooks/src/check_update.ts",
    "install-update": "deno run -q --config=deno.jsonc --allow-run --allow-read --allow-write --allow-net path/to/deno-slack-hooks/src/install_update.ts",
    "doctor": "deno run -q --config=deno.jsonc --allow-read --allow-net path/to/deno-slack-hooks/src/doctor.ts"
  }
}
  1. Import the import { WebClient } from "npm:@slack/web-api";
  2. The app should bundle properly, but it will fail at runtime since @slack/web-api requires --allow-sys=osRelease to work.

Requirements (place an x in each [ ])

@WilliamBergamin WilliamBergamin added the dependencies Pull requests that update a dependency file label Aug 28, 2024
@WilliamBergamin WilliamBergamin self-assigned this Aug 28, 2024
@WilliamBergamin WilliamBergamin requested a review from a team as a code owner August 28, 2024 17:52
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.31%. Comparing base (29c0527) to head (2fd4c5d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/install_update.ts 60.00% 2 Missing ⚠️
src/build.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
- Coverage   78.56%   78.31%   -0.26%     
==========================================
  Files          17       16       -1     
  Lines         877      876       -1     
  Branches      129      130       +1     
==========================================
- Hits          689      686       -3     
- Misses        187      189       +2     
  Partials        1        1              

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

@filmaj
Copy link
Contributor

filmaj commented Aug 28, 2024

The bundling works, but I think this PR now requires exposing env vars to deno permissions; if I run e.g. slak -v manifest from an app that uses this branch of hooks I get:

Requires env access to "DENO_REGISTRY_URL", run again with the --allow-env flag
const JSR_REGISTRY_URL = Deno.env.get("DENO_REGISTRY_URL") ?? "https://jsr.io";

The slack.json for many of our samples don't expose the --allow-env permission. ROSI specifically denies it (and likely we won't be able to adjust that), though ROSI only leverages the deno-slack-runtime project and not the -hooks project. Maybe we can get away with simply exposing --allow-env in samples' slack.json? Not sure.

@WilliamBergamin
Copy link
Contributor Author

@filmaj I've updated this PR to get ride of the deps.ts file. This allows each hook to only import the dependencies it needs to run, instead of all the dependencies in the project.

This also allows Deno to properly restrict system access like --allow-env

@filmaj
Copy link
Contributor

filmaj commented Aug 28, 2024

Sounds good, tomorrow morning I'll run a few tests on a few different apps - fresh and existing - to see how it runs.

src/mod.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM! Tested it out with a new app as well as existing app. Note that, assuming ROSI OKs access to this system API, the start hook and deno-slack-runtime also will need updates to allow access to the system osRelease API.

src/mod.ts Outdated Show resolved Hide resolved
@WilliamBergamin WilliamBergamin merged commit 5b31223 into main Aug 29, 2024
5 of 6 checks passed
@WilliamBergamin WilliamBergamin deleted the update-deps branch August 29, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants