feat: typescript rewrite #560
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erunion
added
enhancement
4 tasks
kanadgupta
reviewed
Aug 9, 2022
| @@ -20,7 +20,7 @@ module.exports = async function getCategories(key, selectedVersion) { | |||
| }), | |||
| }) | |||
| .then(res => { | |||
| totalCount = Math.ceil(res.headers.get('x-total-count') / 20); | |||
| totalCount = Math.ceil(parseInt(res.headers.get('x-total-count'), 10) / 20); | |||
Comment on lines
136
to
137
| * @param {String} folderToSearch path to directory | ||
| * @returns {String[]} array of files |
There was a problem hiding this comment.
Not sure if we need these types I wrote in the JSDocs now 😅
Saw the following error:
```
jest-haste-map: Haste module naming collision: rdme
The following files share their name; please adjust your hasteImpl:
* <rootDir>/dist/package.json
* <rootDir>/package.json
```
this is a fix for that, stolen from here: jestjs/jest#8114 (comment)
kanadgupta
reviewed
Aug 9, 2022
| /** | ||
| * Because our command classes have a `run` method that might not always call `this` we need to | ||
| * explicitly exclude `run` from this rule. | ||
| */ | ||
| "class-methods-use-this": ["error", { "exceptMethods": ["run"] }], | ||
|
|
||
| "import/order": ["error", { |
this was bugging me in the diff lol
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🧰 Changes
After marinating on #558 over the weekend I decided that it'd be best for the codebase if we finally migrated it over to TypeScript before making any ESM decisions.
Commandclass that every command extends from.versions.test.tsis nowversions/index.test.ts, versions/create.test.ts, etc`)🐛 Known Issues
The mainindex.test.tstest is failing.There's some funk with thetypings.d.tsfile. Might need to move those into thetsconfig.jsonsinkconfig I have set up. I had a lot of problems withtypings.d.tsfiles over inapitoo.npm run buildand then./bin/rdmeno commands show up. I think this is because the way the help tool work is that it looks atsrc/cmds, which doesn't exist in thedist/directory. There was a similar problem in feat: refactoring the codebase to be ESM #558 around this dynamic command setup and it might be worth replacing it with asrc/cmds/index.tsfile that loads and exports every available command.~~if no key throw an errorflow in each command by moving that into theCommand.run()superbut strangely promises that would get rejected fromsuper.run()would immediately throw an uncaught promise exception. I'm not sure why, but figuring this out would remove a handful of boilerplate from every command that requires auth.🧬 QA & Testing