Add code#1
Conversation
There was a problem hiding this comment.
looks good. i've left some feedback inline that i hope you will consider. i've tried to avoid repeating comments so please assume they apply everywhere. i think my main concern is all the dependencies this uses. if we could trim this down to be more plain Node.js and TypeScript and not much else, i'd love it ❤️
| { | ||
| "[typescript]": { | ||
| "editor.defaultFormatter": "esbenp.prettier-vscode" | ||
| }, |
There was a problem hiding this comment.
if you add this, i think you should add an extensions.json to recommend the extension and if we're going to do that, we might want to add eslint with promise linting.
There was a problem hiding this comment.
i like that you've broken the template into features and even documented how to delete those features. however, the template is so featureful that users are going to either have to delete a substantial part of it to pare it down to their use case or just drag it along. should we trim this to the bare essentials we want to scaffold for users and leave the rest to the docs and examples? not a blocker for me but my preference would be to provide thin templates with clear structure and not much else.
| @@ -0,0 +1,29 @@ | |||
| // Escape regex metacharacters so moderator-entered text is treated literally. | |||
| const escapeRegExp = (value: string) => | |||
There was a problem hiding this comment.
i suggest adding return types to functions if they're at file scope to clarify and verify intent. this is what we've done for most of the platform.
i also recommend using functions over const since you can order them however you like.
🔕 we'll be able to use RegExp.escape() when we upgrade the backend images.
| OnCommentSubmitRequest, | ||
| OnPostSubmitRequest, | ||
| } from '@devvit/web/shared'; | ||
| import type { T1, T3 } from '@devvit/shared-types/tid.js'; |
There was a problem hiding this comment.
please drop the shared-types dependency and use @devvit/web/shared only. we're trying to limit Devvit dependencies to devvit and @devvit/web.
| // Devvit payload IDs may arrive with or without fullnames. | ||
| // These helpers normalize to the typed IDs required by reddit.get*ById methods. | ||
| const asT1 = (id: string): T1 => (id.startsWith('t1_') ? id : `t1_${id}`) as T1; | ||
| const asT3 = (id: string): T3 => (id.startsWith('t3_') ? id : `t3_${id}`) as T3; |
There was a problem hiding this comment.
i believe we have these exposed under @devvit/web/shared.
| const shouldRemove = props.remove; | ||
| const skipDistinguished = props.skipDistinguished; | ||
|
|
||
| try { |
There was a problem hiding this comment.
please limit the try block to just the parts expected to throw. there's a lot going on in here.
| console.info( | ||
| `${comments.length} comment(s) handled in ${timeElapsed} seconds.` | ||
| ); | ||
| } catch (err: unknown) { |
There was a problem hiding this comment.
i believe unknown is the default in the current version of TS.
| import type { FormField } from '@devvit/shared-types/shared/form.js'; | ||
|
|
||
| // Build form fields shown when moderators invoke a Mop action. | ||
| export const buildNukeFields = (targetId: string): FormField[] => [ |
There was a problem hiding this comment.
i didn't understand why this was a function.
| // Return toast-friendly response for Reddit client. | ||
| return { | ||
| showToast: `${result.success ? 'Success' : 'Failed'} : ${result.message}`, | ||
| } satisfies UiResponse; |
There was a problem hiding this comment.
this works but i'd prefer we drop the satisfies and just type our return values.
| @@ -0,0 +1,102 @@ | |||
| import type { UiResponse } from '@devvit/web/shared'; | |||
| import { context } from '@devvit/web/server'; | |||
| import { isT1, isT3 } from '@devvit/shared-types/tid.js'; | |||
There was a problem hiding this comment.
please get this from @devvit/web/shared.
2 participants
Closes #
💸 TL;DR
📜 Details
Design Doc
Jira
🧪 Testing Steps / Validation
✅ Checks