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

Remove package CDNs from outgoingDomains for the deployed app #32

Closed
wants to merge 2 commits into from

Conversation

zimeg
Copy link
Member

@zimeg zimeg commented Oct 24, 2023

Type of change

  • New sample
  • New feature
  • Bug fix
  • Documentation

Summary

This PR includes cdn.skypack.dev in the outgoingDomains 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

  • This might cause additional confusion but is a workaround for npm: specifiers for packages without using a CDN for now. No hard feelings if this approach isn't favored!
  • Comparing Deno.env.get("SLACK_ENV") === "deployed" instead of Deno.env.get("SLACK_ENV") === "local" here to fallback to including the outgoing domain if the environment isn't specified.

Requirements

  • I’ve checked my submission against the Samples Checklist to ensure it complies with all standards
  • I have ensured the changes I am contributing align with existing patterns and have tested and linted my code
  • I've read and agree to the Code of Conduct

@zimeg zimeg added enhancement New feature or request question Further information is requested labels Oct 24, 2023
@zimeg zimeg self-assigned this Oct 24, 2023
@filmaj
Copy link
Member

filmaj commented Oct 25, 2023

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?

Copy link
Member

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

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.

@filmaj
Copy link
Member

filmaj commented Oct 26, 2023

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

@filmaj
Copy link
Member

filmaj commented Oct 26, 2023

@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 npm: specifiers. So instead of any transpilers, I think we for this sample we can:

  • Remove all outgoing domains from the manifest
  • As part of the upcoming release 42, bump this sample's deno-slack-hooks dependency to the upcoming new version
  • Use npm:@slack/[email protected] for the import URL for the package in question

That should fix this issue! Specifically:

  • in local run mode, npm: specifiers work fine on their own with no changes
  • when deploying, the new deno-slack-hooks version will land support for npm: specifiers

That should cover us in all situations.

@zimeg
Copy link
Member Author

zimeg commented Oct 26, 2023

@filmaj bless the npm: specifier! That sounds like a great solution! I'm good with waiting for that release to swap things over so I'll close this now. Appreciate all of your help here and big thanks for the esbuild updates @WilliamBergamin

@zimeg zimeg closed this Oct 26, 2023
@WilliamBergamin WilliamBergamin mentioned this pull request Oct 26, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants