New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start seriously converting the CLI to TypeScript #743

Merged
merged 7 commits into from Jan 9, 2018

Conversation

Projects
None yet
2 participants
@pimterry
Member

pimterry commented Dec 21, 2017

This PR converts almost all the utils, a little of the easiest commands & all the documentation generation pipeline to TypeScript. This depends on resin-io/resin-sdk#448 and resin-io-modules/stream-logger#4, so won't build until those are complete (though it does build locally for me, all linked together).

I've skipped the Docker utils (since I think @dfunckt is creating a lot of conflicting changes in those as we speak), and I haven't started on the top-level actions etc for now, because that'll be both very conflicty, fairly tricky, this would be hard to review all in one go!

This is still pretty big though - might be a little easier to review commit by commit.

I've tried to be pretty cautious sticking with existing behaviour here for now, and just directly translating, to keep this manageable. That said, there's a two notable discrepancies that I've fixed where TS found real bugs (which is cool):

  • Our current doc generation rendered options with multiple aliases incorrectly, e.g. the --email/-e/-u argument to resin login: https://github.com/resin-io/resin-cli/blob/master/doc/cli.markdown#login.
  • getManifest in helpers.coffee was falling back to the API 100% of the time, because it assumed rindle had already been promisified, tried to call extractAsync (which doesn't exist), and then silently checked the API if it failed.

There's also a fun new TS pattern I'm using here: dynamic import expressions. Lots of CLI code runs require inside functions (to lazily load modules), which TS (and ES6 modules) don't allow.

Instead, we use import() to do this async, plus async functions to make that nice and readable, and a couple of cases where we have to import the types only seperately outside the function to make it work. Nicest solution I can find, but suggestions welcome.

@pimterry pimterry requested review from thgreasi, jviotti and MoranF Dec 21, 2017

@resin-io-versionbot

This comment has been minimized.

Show comment
Hide comment
@resin-io-versionbot

resin-io-versionbot bot Dec 21, 2017

Contributor

@pimterry, status checks have failed for this PR. Please make appropriate changes and recommit.

Contributor

resin-io-versionbot bot commented Dec 21, 2017

@pimterry, status checks have failed for this PR. Please make appropriate changes and recommit.

@pimterry pimterry requested review from lekkas and CameronDiver Dec 21, 2017

Show outdated Hide outdated extras/capitanodoc/index.ts Outdated
Show outdated Hide outdated extras/capitanodoc/index.ts Outdated
Show outdated Hide outdated extras/capitanodoc/markdown.ts Outdated
Show outdated Hide outdated lib/utils/helpers.ts Outdated
Show outdated Hide outdated lib/utils/patterns.ts Outdated
Show outdated Hide outdated lib/utils/patterns.ts Outdated
Show outdated Hide outdated lib/utils/patterns.ts Outdated
Show outdated Hide outdated lib/utils/patterns.ts Outdated
Show outdated Hide outdated lib/utils/config.ts Outdated
Show outdated Hide outdated lib/utils/logger.ts Outdated
@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Jan 4, 2018

Member

Updated to handle pretty much all the comments I think, and rebased. I've also added TS & CS linting with the new resin-lint TS support, which solved a few comments en route and should make things a little tidier generally.

Member

pimterry commented Jan 4, 2018

Updated to handle pretty much all the comments I think, and rebased. I've also added TS & CS linting with the new resin-lint TS support, which solved a few comments en route and should make things a little tidier generally.

@pimterry pimterry requested a review from thgreasi Jan 4, 2018

@resin-io-versionbot resin-io-versionbot bot merged commit 29145df into master Jan 9, 2018

7 checks passed

AutoMerges PR merging is in progress
Reviewers 1/1 review approvals met
Versionist Found all required commit footer tags
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@resin-io-versionbot resin-io-versionbot bot deleted the convert-to-ts branch Jan 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment