Convert scripts to typescript #254
Conversation
G-Rath
added
the
Type: Maintenance
these all sound like great changes, thank you! I'm looking through it now. But I'd like @wolfy1339 and @oscard0m to give their thumbs up for this change before we move forward |
|
No problem! I can break a few of these changes out into another PR if you'd like (specifically, removing |
gr2m
left a comment
gr2m
left a comment
There was a problem hiding this comment.
just a few comments from me, thanks for all your help!
There was a problem hiding this comment.
- moving all the scripts to TypeScript - this uncovered a few small nits, and also revealed that
getSectionsdoesn't need to be async which is cool 🎉
Amazing Job! Thanks for your time and knowledge! It has been a pleasure reading your TS part 🤓
replacing
fs-extrawith native methods - most of the methods were passthroughs fromfs, and the read & write forJSONwasn't being used
- I did replace the use of
existswith atry/catch- fs existence checks are usually not recommended as they're technically a race condition, so the throw could always happen
Didn't know about this. Adding a link just in case somebody want to take a look into it> https://nodejs.org/api/fs.html#fs_fs_exists_path_callback
- tbh I think its probably worth just inlining
cacheintoget-htmlas thats the only place it's used
Yep, to me has sense to unify files :)
- removed
execadev dependency as its unused
✅
- updating the workflows to all use the same node & action versions (node 12)
🥳 🍾
- ran
sort-package-jsonfor consistency (happy to revert this if people want - I just find it nice to not have to worry about the order being consistent)
✅ To me it's fine. Maybe we can open an issue to automate this step. What do you think? @G-Rath @gr2m @wolfy1339. I found this Prettier plugin for instance https://github.com/matzkoh/prettier-plugin-packagejson
💬 Left a comment regarding tsconfig.json. If you want to move forward and open a different issue to tackle that to me it's completely fine.
A general feedback:
-
🥇 Amazing all the job you did here and other PR's. Congratulations and thanks for your effort and for sharing your knowledge with the community.
-
💬 Personally as a contributor and as a reviewer I would prefer to get more PR's with a single purpose or change (just a personal preference 😃). It's easier to read and review, easy to categorize when using semantic versioning and, in case of a regression, easier to revert too. What are your thoughts on this @G-Rath ?
| { | ||
| "compilerOptions": { | ||
| "target": "es2019", | ||
| "module": "commonjs", | ||
| "moduleResolution": "node", | ||
| "lib": [ "es2020" ], | ||
| "rootDir": "./", | ||
| "allowJs": true, | ||
| "checkJs": true, | ||
| "noEmit": true, | ||
| "strict": true, | ||
| "resolveJsonModule": true, | ||
| "useDefineForClassFields": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "noImplicitReturns": true, | ||
| "noFallthroughCasesInSwitch": true, | ||
| "noEmitOnError": true, | ||
| "removeComments": false, | ||
| "esModuleInterop": true | ||
| }, | ||
| "exclude": ["cache", "coverage", "node_modules"], | ||
| "include": ["**/*.js", "**/*.ts"] | ||
| } |
There was a problem hiding this comment.
Would it be possible to extend from https://github.com/octokit/tsconfig? We have some repositories pending to migrate to extend this tsconfig
|
@oscard0m thanks for such an awesome write-up!
Couldn't agree more - that ones totally on me! Usually I would've done it like that, but all the rebasing and creation/deletion of so many files very quickly tends to upset my file system (specifically the WSL-based watchers) 😬
No problem - thanks for being so engaging, it makes the contributions feel a lot more valuable. Feel free to ping me elsewhere if you'd like a hand or have questions, especially with Typescript-related stuff 😄 (that goes for @wolfy1339 & @gr2m too - you've all been really fun to work with!)
Can do! The prettier plugin you linked immediately came to mind - I'll set it up in a follow up PR. Since this PR is branched off #252, I'll apply the changes we've discussed to this branch while that PR is still open, and then will open new PRs for whatever ones I don't get around to addressing when its merged :) |
|
no worries, PRs can be as messy as it makes sense. We can squash all the commits into a single one at the time of the merge, no need for rebases |
|
I was meaning using "Rebase and Merge" rather than "Squash and Merge" into the base branch, since these commits are pretty unrelated to each other and so squashing them into a single commit would... well have them as a single big commit 😅 |
that's all right, we still have the separate commits in the PR. Don't worry too much about it. If you like, we can do a rebase with cleaned up commits, but I think it's not the most fun thing to do ;) we'll be fine |
|
Since #252 has been merged, this PR can be landed - I didn't get around to addressing the extras, so I'll do them in follow-up PRs. |
|
🎉 This PR is included in version 3.32.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
This is branched off #252 to avoid me having to do an ugly rebase - ideally that should be merged first to make this review-able
What started as converting the scripts to TS ballooned slightly, so this includes a few other maintenance improvements:
getSectionsdoesn't need to be async which is cool 🎉fs-extrawith native methods - most of the methods were passthroughs fromfs, and the read & write forJSONwasn't being usedexistswith atry/catch- fs existence checks are usually not recommended as they're technically a race condition, so the throw could always happencacheintoget-htmlas thats the only place it's usedexecadev dependency as its unusedsort-package-jsonfor consistency (happy to revert this if people want - I just find it nice to not have to worry about the order being consistent)