-
Notifications
You must be signed in to change notification settings - Fork 5
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
Remove package CDNs from outgoingDomains
for the deployed app
#32
Conversation
I don't like this conditional approach as we're diverging behaviour based on local run or deployed environments. It feels kind of sneaky to have an app that, when in local mode, would ask an admin to approve the app installation and have NO outgoing domains, but in deployed mode do something different. Also, what dependencies in this repo require the use of the transpiler? I couldn't find any references to npm packages but maybe I'm blind? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is OK for now - but - we should add a TODO here to remove this hack (and I say that word with the utmost respect).
Another change I would suggest here is to swap out skypack for esm.sh, mainly because deno explicitly mentions this CDN in their docs on npm interop. @zimeg are you able to test out esm.sh and see if it works here?
If we can all agree to converge on a single CDN for this kind of stuff, like esm.sh, then I think the next step would be to just implicitly approve esm.sh when running locally, as per our internal discussion. I've filed an issue in runtime for this.
@zimeg I just tried using esm.sh instead of cdn.skypack.dev together with this PR: slackapi/deno-slack-runtime#57 Works well, and no manifest changes needed. |
@zimeg OK one more thing here! After some discussion with @WilliamBergamin, Will brought up a good point that the next release of deno-slack-hooks will include bundling support for
That should fix this issue! Specifically:
That should cover us in all situations. |
@filmaj bless the |
Type of change
Summary
This PR includes
cdn.skypack.dev
in theoutgoingDomains
for only the local app since this package is automatically bundled when packaging the deployed app.The change is made to help expedite app approval processes by removing outgoing domains from the deployed app to make this app fully hosted.
Reviewers
These changes can be verified with both a local and deployed app by running these apps with a scheduled rotation to ensure the rotation happens.
The change to outgoing domains in the manifest can be checked by viewing the manifest for the corresponding app IDs here: https://api.slack.com/methods/apps.manifest.export/test
Notes
npm:
specifiers for packages without using a CDN for now. No hard feelings if this approach isn't favored!Deno.env.get("SLACK_ENV") === "deployed"
instead ofDeno.env.get("SLACK_ENV") === "local"
here to fallback to including the outgoing domain if the environment isn't specified.Requirements