Skip to content
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

Complete refactoring #2

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ tmp/
.yalc/
yalc.lock
.eslintcache

5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,8 @@
},
"workspaces": [
"packages/*"
]
],
"dependencies": {
"rimraf": "^3.0.0"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to dev dependencies

}
}
4 changes: 3 additions & 1 deletion packages/botonic-cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
"@oclif/config": "^1",
"@oclif/plugin-help": "^2",
"analytics-node": "^3.3.0",
"bestzip": "^2.1.4",
"colors": "^1.2.1",
"folder-hash": "^3.0.0",
"form-data": "^2.3.2",
"fs-extra": "^8.1.0",
"inquirer": "^6.3.1",
"ora": "^3.0.0",
"tslib": "^1"
Expand Down Expand Up @@ -54,7 +56,7 @@
},
"repository": "hubtype/botonic",
"scripts": {
"build": "rm -rf lib && tsc",
"build": "rimraf lib && tsc",
"postpack": "rm -f oclif.manifest.json npm-shrinkwrap.json",
"posttest": "tslint -p test -t stylish",
"prepack": "oclif-dev manifest && oclif-dev readme && npm shrinkwrap",
Expand Down
4 changes: 2 additions & 2 deletions packages/botonic-cli/src/botonicAPIService.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { join, resolve } from 'path'
import * as fs from 'fs'
import { homedir } from 'os'
import axios from 'axios'
import axios, {Method} from 'axios'
import * as colors from 'colors'
const FormData = require('form-data')
const util = require('util')
Expand Down Expand Up @@ -152,7 +152,7 @@ export class BotonicAPIService {
async api(
path: string,
body: any = null,
method: string = 'get',
method: Method = 'get',
headers: any | null = null,
params: any = null
): Promise<any> {
Expand Down
56 changes: 34 additions & 22 deletions packages/botonic-cli/src/commands/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,17 @@ import { prompt } from 'inquirer'
import * as colors from 'colors'

const fs = require('fs')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use fs-extra for all file system operations and rename next line from fsExtra to fs - every method from fs is supported by fs-extra, so you can use it as a drop-in replacement.

const fsExtra = require('fs-extra')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really related to this line but rather whole block - notice that there are import statements above it, but below that there are requires.... I think it should be consistent (you can use imports everywhere - it's TypeScript), but there are a lot of other code quality issues that linter would report so I'm not sure if you should be handling that or not.

const ora = require('ora')
const util = require('util')
const zip = require('bestzip')
const exec = util.promisify(require('child_process').exec)

import { BotonicAPIService } from '../botonicAPIService'
import { track, sleep } from '../utils'
import { sleep, track } from '../utils'

var force = false
var npmCommand: string | undefined
let force = false
let npmCommand: string | undefined

export default class Run extends Command {
static description = 'Deploy Botonic project to hubtype.com'
Expand All @@ -38,10 +40,10 @@ Uploading...

static args = [{ name: 'bot_name' }]

private botonicApiService: BotonicAPIService = new BotonicAPIService()
private readonly botonicApiService: BotonicAPIService = new BotonicAPIService()

async run() {
const { args, flags } = this.parse(Run)
const { flags } = this.parse(Run)
track('Deployed Botonic CLI')

force = flags.force ? flags.force : false
Expand All @@ -50,7 +52,7 @@ Uploading...

if (!this.botonicApiService.oauth) await this.signupFlow()
else if (botName) {
this.deployBotFromFlag(botName)
await this.deployBotFromFlag(botName)
} else await this.deployBotFlow()
}

Expand All @@ -62,13 +64,13 @@ Uploading...
await this.botonicApiService.getMoreBots(bots, nextBots)
}
let bot = bots.filter(b => b.name === botName)[0]
if (bot == undefined) {
if (bot === undefined) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really fishy to be honest... Generally bot should always return either some kind of "bot" object or undefined but you don't really check undefined like that ever. And here we actually only want this bot object. I'd suggest switching contents of if and else blocks and use if (bot).

console.log(colors.red(`Bot ${botName} doesn't exist.`))
console.log('\nThese are the available options:')
bots.map(b => console.log(` > ${b.name}`))
} else {
this.botonicApiService.setCurrentBot(bot)
this.deploy()
await this.deploy()
}
}

Expand All @@ -83,10 +85,10 @@ Uploading...
name: 'signupConfirmation',
message:
'You need to login before deploying your bot.\nDo you have a Hubtype account already?',
choices: choices
choices
}
]).then((inp: any) => {
if (inp.signupConfirmation == choices[1]) return this.askLogin()
if (inp.signupConfirmation === choices[1]) return this.askLogin()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal preference but I don't really like inline if/else, you could replace this whole block with ternary.

else return this.askSignup()
})
}
Expand Down Expand Up @@ -120,8 +122,9 @@ Uploading...
}

async deployBotFlow() {
if (!this.botonicApiService.bot) return this.newBotFlow()
else {
if (!this.botonicApiService.bot) {
return this.newBotFlow()
} else {
let resp = await this.botonicApiService.getBots()
let nextBots = resp.data.next
let bots = resp.data.results
Expand All @@ -131,7 +134,7 @@ Uploading...
// Show the current bot in credentials at top of the list
let first_id = this.botonicApiService.bot.id

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is everything in camelCase (or so I thought) and this thing is in snake_case? Again not sure if you should even care about this but yeah...

bots.sort(function(x, y) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use arrow function

return x.id == first_id ? -1 : y.id == first_id ? 1 : 0
return x.id === first_id ? -1 : y.id === first_id ? 1 : 0
})
return this.selectExistentBot(bots)
}
Expand Down Expand Up @@ -268,8 +271,15 @@ Uploading...
text: 'Creating bundle...',
spinner: 'bouncingBar'
}).start()
let zip_cmd = `zip -r botonic_bundle.zip dist`
let zip_out = await exec(zip_cmd)
try {
await zip({
source: 'dist/*',
destination: './botonic_bundle.zip'
})
} catch(err) {
console.log(colors.red(err))
return
}
const zip_stats = fs.statSync('botonic_bundle.zip')
spinner.succeed()
if (zip_stats.size >= 10 * 10 ** 6) {
Expand All @@ -280,15 +290,15 @@ Uploading...
)
)
track('Deploy Botonic Zip Error')
await exec('rm botonic_bundle.zip')
fsExtra.remove('botonic_bundle.zip').catch(err => console.log(colors.red(err)))
return
}
spinner = new ora({
text: 'Deploying...',
spinner: 'bouncingBar'
}).start()
try {
var deploy = await this.botonicApiService.deployBot(
let deploy = await this.botonicApiService.deployBot(
'botonic_bundle.zip',
force
)
Expand All @@ -315,7 +325,7 @@ Uploading...
console.log(colors.red('There was a problem in the deploy:'))
console.log(deploy_status.data.error)
track('Deploy Botonic Error', { error: deploy_status.data.error })
await exec('rm botonic_bundle.zip')
await fsExtra.remove('botonic_bundle.zip')
return
}
}
Expand All @@ -325,7 +335,7 @@ Uploading...
console.log(colors.red('There was a problem in the deploy:'))
console.log(err)
track('Deploy Botonic Error', { error: err })
await exec('rm botonic_bundle.zip')
fsExtra.remove('botonic_bundle.zip').catch(err => console.log(colors.red(err)))
return
}
try {
Expand All @@ -339,15 +349,17 @@ Uploading...
}`
console.log(links)
} else {
this.displayProviders(providers)
await this.displayProviders(providers)
}
} catch (e) {
track('Deploy Botonic Provider Error', { error: e })
console.log(colors.red(`There was an error getting the providers: ${e}`))
}
try {
await exec('rm botonic_bundle.zip')
} catch (e) {}
await fsExtra.remove('botonic_bundle.zip')
} catch(err) {
console.log(colors.red(err))
}
this.botonicApiService.beforeExit()
}
}
22 changes: 15 additions & 7 deletions packages/botonic-cli/src/commands/new.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { Command } from '@oclif/command'
import { resolve } from 'path'
import { prompt } from 'inquirer'
import * as colors from 'colors'
import { prompt } from 'inquirer'

import { BotonicAPIService } from '../botonicAPIService'
import { track } from '../utils'

const util = require('util')
const ora = require('ora')
const exec = util.promisify(require('child_process').exec)
const fsExtra = require('fs-extra')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to fs


export default class Run extends Command {
static description = 'Create a new Botonic project'
Expand Down Expand Up @@ -70,7 +71,7 @@ Creating...

async run() {
track('Created Botonic Bot CLI')
const { args, flags } = this.parse(Run)
const { args } = this.parse(Run)
let template = ''
if (!args.templateName) {
await this.selectBotName().then((resp: any) => {
Expand All @@ -85,7 +86,6 @@ Creating...
if (botExists.length) {
template = args.templateName
} else {
let template_names = this.templates.map((t: any) => t.name)
console.log(
colors.red(
'Template ${args.templateName} does not exist, please choose one of ${template_names}.'
Expand All @@ -94,14 +94,17 @@ Creating...
return
}
}
let botPath = resolve(template)
let templatePath = `${__dirname}/../../templates/${template}`
let spinner = new ora({
text: 'Copying files...',
spinner: 'bouncingBar'
}).start()
let copyFolderCommand = `cp -r ${templatePath} ${args.name}`
let copy_out = await exec(copyFolderCommand)
try {
await fsExtra.copy(`${templatePath}`, `${args.name}`)
} catch (err) {
console.log(colors.red(err))
return
}
spinner.succeed()
process.chdir(args.name)
spinner = new ora({
Expand All @@ -113,7 +116,12 @@ Creating...
spinner.succeed()
await this.botonicApiService.buildIfChanged(false)
this.botonicApiService.beforeExit()
await exec('mv ../.botonic.json .')
try {
await fsExtra.move('../.botonic.json', './botonic.json')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's a typo or not, it's .botonic.json to botonic.json without dot at the beginning but in the original it just moves the file one directory up - is that what we want?

} catch (err) {
console.log(colors.red(err))
return
}
let cd_cmd = colors.bold(`cd ${args.name}`)
let run_cmd = colors.bold('botonic serve')
let deploy_cmd = colors.bold('botonic deploy')
Expand Down
2 changes: 1 addition & 1 deletion packages/botonic-cli/templates/tutorial/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
}
},
"dependencies": {
"@botonic/react": "0.9.0"
"@botonic/react": "0.9.0-alpha.7"
},
"devDependencies": {
"babel-jest": "^24.1.0",
Expand Down