chore: update to latests Deno and CLI standards#48
Conversation
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin Solid set of changes here!
I'm approving now because the sample is working well for me with both run and deploy but I am hesitant on the changes to testing utilities included in the sample code.
I left a few comments on this and updates to make certain files more specific to this sample, but these are changes that might be approached in follow up PRs. Please let me know what you think about some of these 👾 ✨
| - package-ecosystem: "github-actions" | ||
| directory: "/" | ||
| schedule: | ||
| interval: "monthly" |
There was a problem hiding this comment.
📣 For sample apps this is an interesting choice that seems helpful for the current maintenance requirements across all projects of @slack-samples.
There was a problem hiding this comment.
Changes to this file might be reverted from automations of slack-samples/sample-app-configurations but I realize the changed imports perhaps make that automation not useful now?
Either before or after this pull request we should update these workflows! 🤖
There was a problem hiding this comment.
Rambling now, but if we do expect these files to diverge I think it'd be best to remove paths that don't exist within a specific sample and perhaps revisit the lock file? 🔏
I'm also noticing sections in the README.md exist for paths not found in this sample. It might be best to hold off on making these files more sample specific than import updates within these changes...
There was a problem hiding this comment.
Yess I agree 💯 and I was also thinking of updating slack-samples/sample-app-configurations to use imports rather then import maps
deno.jsonc
Outdated
| "@std/testing": "jsr:@std/testing@^1.0.12", | ||
| "@std/assert": "jsr:@std/assert@^1" |
There was a problem hiding this comment.
QQ: Do we want to use pinned patches for both of these packages? I'm optimistic of future changes to automatic updates, but I'm also confident that these stable versions should not break.
There was a problem hiding this comment.
Yeah the stable versions might be our best option I haven't really given it some thought but since I'm here maybe we should
| @@ -1,4 +1,4 @@ | |||
| import { Trigger } from "deno-slack-sdk/types.ts"; | |||
| import type { Trigger } from "deno-slack-sdk/types.ts"; | |||
functions/test_utils.ts
Outdated
There was a problem hiding this comment.
📝 This has complex functionalities IMO that might be confusing to include in a sample without more comments. I'm partial to wanting to avoid utils overall too. No blocker, but it'd be helpful to have comments here for now!
Also this might be a useful function to include with deno-slack-api because stubbing requests and mocking responses from https://slack.com/api is a common test case that can be useful across samples.
There was a problem hiding this comment.
I believe this might be the most complex implementation of stubbing fetch across samples, other sample should not require this level of complexity, adding comments is a good idea because this is a sample!
Co-authored-by: Eden Zimbelman <zim@o526.net>
Co-authored-by: Eden Zimbelman <zim@o526.net>
test_utils.ts
Outdated
There was a problem hiding this comment.
🌲 I forget exact testing conventions with filenames but could this be made into a separate test directory?
Perhaps as: ./test/http.ts 🤖
There was a problem hiding this comment.
I'm not a fan of where this lives either 💯 moving to ./test/http.ts feels better
There was a problem hiding this comment.
Scrapped all this and opted for an explicit stubbing pattern
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin LGTM! And thanks for exploring alternate methods of stubs and tests.
I feel a bit smarter having learned about these approaches, but agree that the inlined mocks make both the diff smaller and setups more obvious at a glance 🤓
Type of change
Summary
This PR aims to update the sample to the Deno 2 standards and the latest CLI standards.
slack.jsonto.slack/hooks.jsonmock-fetch/import-map.jsontodeno.jsoncRequirements