feat(version-bump): replace inline bash with typescript#19
Conversation
0f595d5 to
5cac193
Compare
| head: releaseBranch, | ||
| }); | ||
|
|
||
| await octokit.graphql( |
There was a problem hiding this comment.
Nit: this is a bit hard to review. Are you able to break this out into a graphql file?
Not required, but nicer to look at if this is easy.
There was a problem hiding this comment.
I think I agree with this. Honestly we should be getting into the practice of basically never in-lining things as much as possible. I may have been the genesis of this. I also need to be better about it
There was a problem hiding this comment.
most of the time i would agree with this but it seems the docs on using octokit generally use inline graphql
There was a problem hiding this comment.
JK. Adriana and I talked about this and the right way to do this would be type gen'ing and pulling in the GraphQL types that way. I the cost/benefit to doing that for something this small seems lop-sided in the cost direction.
There was a problem hiding this comment.
+1. the other alternative is to have a string export in a different type file that exports like export const ENABLE_AUTO_MERGE = 'mutation ...';... at which point it's no different than just declaring a const string here 🤷
yankrasny
left a comment
yankrasny
left a comment
There was a problem hiding this comment.
This looks good and is an improvement of the code. Is there a ticket we can link so that the goal is clearer? The description just says new stuff, but why?
GoogilyBoogily
left a comment
GoogilyBoogily
left a comment
There was a problem hiding this comment.
Two nits. I'll leave them as comments and if we decide they're not worth changing, I'll approve!
| head: releaseBranch, | ||
| }); | ||
|
|
||
| await octokit.graphql( |
There was a problem hiding this comment.
I think I agree with this. Honestly we should be getting into the practice of basically never in-lining things as much as possible. I may have been the genesis of this. I also need to be better about it
| }, | ||
| "overrides": { | ||
| "undici": "^8.3.0" | ||
| "undici": "^6.23.0" |
There was a problem hiding this comment.
Taking a second look at this. Does this need to be in the overrides section anymore now that it's matching the version in the other package?
There was a problem hiding this comment.
oh you're probably right. i didn't think about that hah. i guess it could probably just move to the regular dependencies section now.
i think the original reason for it being in the overrides is bc pinning it reduces the bundle size to reduce duplications. so i think it makes sense to stay an override
5cac193 to
55d051e
Compare
55d051e to
a7e8e1c
Compare
4 participants
new stuff:
small tweaks: