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

Support npm specifiers #33

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Support npm specifiers #33

merged 3 commits into from
Apr 8, 2024

Conversation

WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented Oct 26, 2023

Type of change (place an x in the [ ] that applies)

  • New sample
  • New feature
  • Bug fix
  • Documentation

Summary

This PR aims to update the version of the deno-slack-hooks package and enable the project to use npm specifiers to import packages

This also resolves #32

Testing

  1. Pull this branch
  2. Run slack deploy
  3. The project should bundle and deploy as expected

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

@WilliamBergamin WilliamBergamin added the bug Something isn't working label Oct 26, 2023
@filmaj
Copy link
Contributor

filmaj commented Oct 26, 2023

Should this be created as a pre-release branch for the upcoming release? Since it includes a freshly-cut SDK version bump, and the automated tests will rely on the existence of a pre-release branch?

@WilliamBergamin
Copy link
Contributor Author

WilliamBergamin commented Oct 26, 2023

this PR or the pre-release PR will update the package, but we may require a follow up PR to update the npm specifiers

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.

Should probably use a versioned types package and also move it into a deps.ts if it exists?

This PR will also conflict with #34 FYI.

@@ -8,7 +8,7 @@ import {
MultiUsersSelect,
Option,
SectionBlock,
} from "https://cdn.skypack.dev/@slack/types?dts";
} from "npm:@slack/types";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd version this, the current version is 2.9.0. I am planning on releasing a 3.0 version soon-ish which will include breaking changes.

@WilliamBergamin WilliamBergamin requested a review from filmaj April 8, 2024 17:46
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 but def should be tested with an actual slack deploy / at least a deno-hooks build hook invocation to see if esbuild can handle it or not.

Would be nice to add that to samples' CI, too: can the project be built using the deno build hook?

@WilliamBergamin
Copy link
Contributor Author

🟢 tested with slack deploy everything works as expected
🟢 tested with deno run ... https://deno.land/x/[email protected]/build.ts everything builds as expected

@WilliamBergamin WilliamBergamin merged commit 03da4d7 into main Apr 8, 2024
2 checks passed
@WilliamBergamin WilliamBergamin deleted the support-npm-specifiers branch April 8, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants