Convert create-cli.js (gatsby-cli) to TypeScript #23650
Conversation
alexpyzhianov
changed the title
alexpyzhianov
changed the title
There was a problem hiding this comment.
- Dynamic require()'s are a pain in the rear. We have no way of knowing what we've required, except that it's a function.
It is pain for sure, but maybe we can do a bit to make it somewhat typed? In gatsby package there is making of interface for those types - https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/commands/types.ts#L8-L23 (interface we would pass from cli would be smaller? or maybe the types we have are totally wrong - joys of migration). Also of course depending on command - we will not have single interface - rather we would have some base and things that expand it for specific commands.
I do think that that things we pass from gatsby-cli should be defined in gatsby-cli and just imported in gatsby core package
- Giving proper type annotations to each and every cli command looks like a lot of work and might not prove very useful anyway as the local commands have no type signature anyway.
It wouldn't be useful for typechecking I guess, but it makes way more sense to define those in CLI (at least for now, as CLI is only supported way of invoking commands) and import them in our gatsby package?
| @@ -64,11 +64,11 @@ class Reporter { | |||
| /** | |||
| * Log arguments and exit process with status 1. | |||
| */ | |||
| panic = (errorMeta: ErrorMeta, error?: Error | Error[]): void => { | |||
| panic = (errorMeta: ErrorMeta, error?: Error | Error[]): never => { | |||
| .fail((msg, err, yargs) => { | ||
| .fail((msg: string, _err: Error, yargs: any): void => { | ||
| // FIXME: could not find any mention of getCommands in the typings or the docs | ||
| // It might be some private API which should not be used |
There was a problem hiding this comment.
Right, this is apparently private API. Which is a shame, because afaik there is no other way to get this list without maintaining list of commands ourselves separately.
Do you know if we can create such a list and then enforce that each command wi create with .command function is using one of those? (so the list doesn't get out of sync because typechecker would complain about it)
There was a problem hiding this comment.
create such a list and then enforce that each command wi create with .command function is using one of those
Not sure, but worth trying 👍
|
I was thinking about this PR a bit - while as you mentioned it's not perfect - I think it has enough initial typings to get this in without doing a lot of rework stuff that is likely needed to handle problems you outlined. I'm about to start working on upgrading |
|
@pieh 👍ok, got it! I've rebased it onto the fresh master and resolved the draft status. So it ready to be merged if you decide to do so |
| @@ -40,7 +40,7 @@ | |||
| "prompts": "^2.3.2", | |||
| "react": "^16.8.0", | |||
| "redux": "^4.0.5", | |||
| "resolve-cwd": "^2.0.0", | |||
| "resolve-cwd": "^3.0.0", | |||
There was a problem hiding this comment.
Just so there is paper trail (not something to act on):
Breaking changes in 2->3 ( from https://github.com/sindresorhus/resolve-cwd/releases/tag/v3.0.0 )
Breaking:
- Require Node.js 8
- Return undefined instead of null when the module can't be found when using resolveCwd.silent()
Both are good and don't require any changes from us - we already require node 10 and resolveCwd.silent() change doesn't impact us because we just check for falsiness and not concrete values (and both undefined and null are falsy)
|
Hey @alexpyzhianov - I see that you scheduled pairing session to work on this PR. With that in mind I think course of action will be that I will bump |
|
@pieh Thanks 🙏 I was hoping to resolve those next step issues with typing yargs commands and local functions, so it should not affect the current changes, at least in major ways |
|
To make sure not to mess with your branch - in case of conflicts - I will open pull request that resolve conflicts against your fork (just so you know what to expect :) ) |
* Update resolve-cwd to 3.0.0 as it has TS definitions
* Add @types/yargs
* Add most obvious type annotations and checks to create-cli.ts
* Make report.panic return "never" as it's supposed to
NOTE:
This still leaves a lot of any types in create-cli,
especially when it comes to require('./some-local-file')
expressions and parameters/options of yargs commands.
Making everything type-safe might introduce a lot of complexity though,
so one should think about benefits vs costs of doing this
NOTE: gatsby new arguments are now stringified before use. This makes the compiler happy and also it's now possible to init a gastsby project inside a folder with number as a name, in case you want to "gatsby new 42", which caused an error before
|
@blainekasten Thanks a bunch for fixing those failing test cases, I wasn't at all sure about what to do with them 💪 You rock! Everything looks good to me now, what do you think? Anything else we should do? |
blainekasten
added
the
bot: merge on green
|
@alexpyzhianov this was amazing. Had a great time pairing with you! Nice work 💜 |
|
@blainekasten Thanks for an amazing learning experience! I'll try to contribute more for sure 🚀 |
Description
create-cli.js -> create-cli.ts
Because create-cli.js is by no means small or simple, this PR is far from being perfect. I've added type annotations where it was relatively straightforward, but this leaves a lot of blank spots still. In particular:
I would love some feedback from a person who knows the codebase well to figure if my contribution here makes sense and if it's possible to make the PR better.
Documentation
No changes required.
Related Issues
Related to: #21995