-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add bundle progress bar to logs #140
Changes from 6 commits
27357a5
a0755d8
3348855
c38e4b7
daa79e7
ecdf6cb
ccd5933
13c1e5e
4e83841
6a435dd
2f81a8d
6b9fd95
3f316a5
e76536a
1ccb088
6cd268d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,8 +33,9 @@ | |
"match-require": "^2.0.0", | ||
"minimist": "^1.2.0", | ||
"path-exists": "^3.0.0", | ||
"progress": "^1.1.8", | ||
"qrcode-terminal": "^0.11.0", | ||
"xdl": "^37.0.0" | ||
"xdl": "38.0.0-beta.2" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming we'll release this as at-latest before we merge this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup! |
||
}, | ||
"devDependencies": { | ||
"babel-plugin-add-module-exports": "^0.2.1", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,12 +5,13 @@ import fsp from 'fs-promise'; | |
import path from 'path'; | ||
import pathExists from 'path-exists'; | ||
import spawn from 'cross-spawn'; | ||
import log from '../util/log'; | ||
|
||
// UPDATE DEPENDENCY VERSIONS HERE | ||
const DEFAULT_DEPENDENCIES = { | ||
expo: '^15.1.0', | ||
expo: '^16.0.0-beta.0', | ||
react: '~15.4.0', | ||
'react-native': '0.42.3', | ||
'react-native': 'github:expo/react-native#exp-latest', | ||
}; | ||
|
||
// TODO figure out how this interacts with ejection | ||
|
@@ -94,8 +95,8 @@ module.exports = async (appPath: string, appName: string, verbose: boolean, cwd: | |
} | ||
} | ||
|
||
console.log(`Installing dependencies using ${command}...`); | ||
console.log(); | ||
log(`Installing dependencies using ${command}...`); | ||
log(); // why is this here | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because it's ugly otherwise and i apparently forgot how newlines work |
||
|
||
if (command === 'yarnpkg') { | ||
// it's weird to print a yarn alias that no one uses | ||
|
@@ -118,7 +119,7 @@ module.exports = async (appPath: string, appName: string, verbose: boolean, cwd: | |
cdpath = appPath; | ||
} | ||
|
||
console.log( | ||
log( | ||
` | ||
|
||
Success! Created ${appName} at ${appPath} | ||
|
@@ -151,13 +152,13 @@ We suggest that you begin by typing: | |
); | ||
|
||
if (readmeExists) { | ||
console.log( | ||
log( | ||
` | ||
${chalk.yellow('You had a `README.md` file, we renamed it to `README.old.md`')}` | ||
); | ||
} | ||
|
||
console.log(); | ||
console.log('Happy hacking!'); | ||
log(); | ||
log('Happy hacking!'); | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
// @flow | ||
|
||
import chalk from 'chalk'; | ||
|
||
function log() { | ||
const args = Array.prototype.slice.call(arguments, 0); | ||
|
||
respectProgressBars(() => { | ||
console.log(...args); | ||
}); | ||
}; | ||
|
||
log.withTimestamp = function() { | ||
const prefix = chalk.dim(new Date().toLocaleTimeString()) + ':'; | ||
const args = [prefix].concat(Array.prototype.slice.call(arguments, 0)); | ||
|
||
respectProgressBars(() => { | ||
console.log(...args); | ||
}); | ||
} | ||
|
||
let _bundleProgressBar; | ||
log.setBundleProgressBar = function(bundleProgressBar) { | ||
_bundleProgressBar = bundleProgressBar; | ||
}; | ||
|
||
function respectProgressBars(commitLogs) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existence of this function makes me think that we also need to replace console.log's in other scripts, as there's a chance we may use, for example, the Expo login stuff inside of a script that needs to respect progress bars. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about this more, we probably want to replace all of the console.log statements so that there's no confusion about when or where to use this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dikaiosune - so just:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wfm |
||
if (_bundleProgressBar) { | ||
_bundleProgressBar.terminate(); | ||
_bundleProgressBar.lastDraw = ''; | ||
commitLogs(); | ||
_bundleProgressBar.render(); | ||
} else { | ||
commitLogs(); | ||
} | ||
} | ||
|
||
export default log; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue concerns me re: windows support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, I don't have a windows machine to test this :<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or rather I do but I have no idea how to use windows for anything except oculus and netflix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw we use this same ProgressBar module on exp so this could be a problem we have to address there too if it is indeed true and not user error in that issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok. hm maybe it's broken in exp too then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you try out installing exp and init'ing a project? should show progress bar while downloading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wipes tear away while spinning up a VM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so apparently the inquirer package is also irrevocably broken on git bash, so i'm just going to step away slowly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolution: don't support git bash for now if you want a pleasant crna experience. powershell recommended on windows for now. if anyone reading this is interested in fixing git bash support it would be welcome!