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

Convert lots of actions to TypeScript #755

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@pimterry
Member

pimterry commented Jan 11, 2018

Mainly focusing on converting the actions code (this is 1/2 of them, but mainly the smaller ones). Notes:

  • I've split most separate actions into separate commits, which should be fairly easily reviewable individually
  • This also upgrades to TypeScript 2.6.2, and SDK 8 (lots of coffeescript is still using resin-sdk-preconfigured, which is SDK 6, but that's slowwwwly dying)
  • There's a few bug fixes this found - e.g. it looks like ssh proxying never worked because we took a non-promise function and never passed a callback - but in general, this shouldn't be a functional change
  • I've ended up with some really cool stuff in the resin-cli-form types, which is fun. Usage (and maybe slightly the types themselves) might be of interest to @LucianBuzzo, as we start looking at form generation more generally.

@pimterry pimterry requested review from thgreasi, MoranF and LucianBuzzo Jan 11, 2018

@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Jan 11, 2018

Member

After a bug got introduced in the last PR, and since we have near-zero test coverage I thought I'd do some manual testing of the main changed commands:

  • help [command...]
  • login
    • web
    • credentials
    • auth token
    • register (against staging)
  • apps
  • app
  • logs
  • sync [uuid]
  • ssh [uuid]
  • logout
  • signup (against staging)
  • whoami
  • app create
  • app rm
  • app restart
  • note <|note>
  • util available-drives

That's just the basic successful case, so there might be some interesting corner cases, but in general it's looking good!

Member

pimterry commented Jan 11, 2018

After a bug got introduced in the last PR, and since we have near-zero test coverage I thought I'd do some manual testing of the main changed commands:

  • help [command...]
  • login
    • web
    • credentials
    • auth token
    • register (against staging)
  • apps
  • app
  • logs
  • sync [uuid]
  • ssh [uuid]
  • logout
  • signup (against staging)
  • whoami
  • app create
  • app rm
  • app restart
  • note <|note>
  • util available-drives

That's just the basic successful case, so there might be some interesting corner cases, but in general it's looking good!

@resin-io-versionbot

This comment has been minimized.

Show comment
Hide comment
@resin-io-versionbot

resin-io-versionbot bot Jan 12, 2018

Contributor

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

Contributor

resin-io-versionbot bot commented Jan 12, 2018

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

@LucianBuzzo

Some NITs and questions, but overall happy for this to be merged :)

const { resin } = await import('../sdk');
const visuals = await import('resin-cli-visuals');
const applications = await resin.models.application.getAll({

This comment has been minimized.

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

I'm guessing that you've taken the opportunity to make the output a little more concise here?

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

I'm guessing that you've taken the opportunity to make the output a little more concise here?

const container = docker.getContainer(name);
return container
.inspect()

This comment has been minimized.

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

All these docker calls used to be called with the Async suffix. Is this no longer necessary due to the docker-toolbelt update?

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

All these docker calls used to be called with the Async suffix. Is this no longer necessary due to the docker-toolbelt update?

deviceIp: deviceIp!,
name: appName,
outStream: process.stdout,
follow: options['follow'],

This comment has been minimized.

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

NIT: this can be options.follow

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

NIT: this can be options.follow

// hasbin ignores the error-first convention, so we can't promisify automatically
const hasbinAsync = (bin: string) =>
new Promise<boolean>(resolve => hasbin(bin, resolve));

This comment has been minimized.

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

Is there a way that we can avoid using native Promise and bluebird here? one of the other would be great

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

Is there a way that we can avoid using native Promise and bluebird here? one of the other would be great

* if isValid
* console.log('Token is valid!')
*/
export function isTokenValid(sessionToken: string) {

This comment has been minimized.

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

This is pre-existing, but this would be a good time to make the implementation match the TSdoc here. The docs say its return type is Promise<boolean> but its actually more like Promise<boolean | void | ???>

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

This is pre-existing, but this would be a good time to make the implementation match the TSdoc here. The docs say its return type is Promise<boolean> but its actually more like Promise<boolean | void | ???>

@@ -0,0 +1,4 @@
declare module 'bash' {

This comment has been minimized.

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

What a strange and futuristic world we live in!

@LucianBuzzo

LucianBuzzo Jan 12, 2018

Member

What a strange and futuristic world we live in!

@thgreasi

That's great👍
My main question is whether I'm missing something on the direct done() calls.

visuals.table.horizontal(
applications.map(app =>
_.assign(app, {
devices_length: app.owns__device!.length,

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

Is that for future proofing or wasn't it working already?

@thgreasi

thgreasi Jan 15, 2018

Member

Is that for future proofing or wasn't it working already?

primary: true,
async action(_params, options, done) {
const _ = await import('lodash');
const Bluebird = await import('bluebird');

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

I would love to see all bluebird imports as a top level import for consistency, like in lib/actions/local/common.ts.
I guess there is no real performance benefit on importing dynamically, since app.coffee (and many other files) have it as top level import.

@thgreasi

thgreasi Jan 15, 2018

Member

I would love to see all bluebird imports as a top level import for consistency, like in lib/actions/local/common.ts.
I guess there is no real performance benefit on importing dynamically, since app.coffee (and many other files) have it as top level import.

const resinUrl = await resin.settings.get('resinUrl');
console.log(messages.resinAsciiArt);
console.log(`\nLogging in to ${resinUrl}`);

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

Would it make sense to combine those two console.infos? (and maybe the other two later in this fn)

@thgreasi

thgreasi Jan 15, 2018

Member

Would it make sense to combine those two console.infos? (and maybe the other two later in this fn)

${messages.reachingOut}\
`);
done();

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

Does this get called with an error first argument if an exception is thrown in the above code?
Would it make sense to wrap the async function in an async iife and use .nodeify like before or a try catch?
(Same question for the rest done() calls.)

@thgreasi

thgreasi Jan 15, 2018

Member

Does this get called with an error first argument if an exception is thrown in the above code?
Would it make sense to wrap the async function in an async iife and use .nodeify like before or a try catch?
(Same question for the rest done() calls.)

export const dockerPort = 2375;
export const dockerTimeout = 2000;
export function isNotSupervisorContainer(container: Dockerode.ContainerInfo) {

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

I think that there is still one reference of the old method name in

{ selectContainerFromDevice, filterOutSupervisorContainer } = require('./common')

@thgreasi

thgreasi Jan 15, 2018

Member

I think that there is still one reference of the old method name in

{ selectContainerFromDevice, filterOutSupervisorContainer } = require('./common')

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

How about implementing an isSupervisorContainer() method instead of the inverted logic?

@thgreasi

thgreasi Jan 15, 2018

Member

How about implementing an isSupervisorContainer() method instead of the inverted logic?

: await patterns.inferOrSelectDevice();
console.info(`Connecting to: ${selectedUuid}`);
const device = await resin.models.device.get(selectedUuid, {});

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

I'm not sure whether the empty options param is bc the sdk wasn't marking the param as optional, but if that's the case then this is a reminder to remove when the sdk is updated.

@thgreasi

thgreasi Jan 15, 2018

Member

I'm not sure whether the empty options param is bc the sdk wasn't marking the param as optional, but if that's the case then this is a reminder to remove when the sdk is updated.

hasTunnelBin: useProxy ? await hasbinAsync('proxytunnel') : false,
});
if (containerId == null) {

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

Could this be if (!containerId)?

@thgreasi

thgreasi Jan 15, 2018

Member

Could this be if (!containerId)?

@@ -72,7 +73,7 @@ export function authenticate(options: {}): Promise<void> {
}
export function askLoginType() {
return form.ask({
return form.ask<'web' | 'register' | 'token' | 'credentials', 'list'>({

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

How about also having this ('web' | 'register' | 'token' | 'credentials') as a type?

@thgreasi

thgreasi Jan 15, 2018

Member

How about also having this ('web' | 'register' | 'token' | 'credentials') as a type?

@@ -102,7 +100,7 @@ export function osProgressHandler(step: InitializeEmitter) {
if (state.operation.command === 'burn') {
return;
}
console.log(exports.stateToString(state));
console.log(stateToString(state));

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

I guess that this wasn't working before.

@thgreasi

thgreasi Jan 15, 2018

Member

I guess that this wasn't working before.

if (param == null) {
param = {};
}
const { port, isDev } = param;

This comment has been minimized.

@thgreasi

thgreasi Jan 15, 2018

Member

Is there really a chance of getting null param?
Could this be

function createServer({ port, isDev }: { port?: number; isDev?: boolean } = {}) { }

?

@thgreasi

thgreasi Jan 15, 2018

Member

Is there really a chance of getting null param?
Could this be

function createServer({ port, isDev }: { port?: number; isDev?: boolean } = {}) { }

?

@thgreasi

Heads-up for the filterOutSupervisorContainer() refactor!

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