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

Improved the now ls command #1202

Closed
wants to merge 34 commits into from
Closed

Improved the now ls command #1202

wants to merge 34 commits into from

Conversation

rauchg
Copy link
Member

@rauchg rauchg commented Mar 17, 2018

  • Fails upon unknown options or invalid number of arguments (Subcommands should error on extraneous options or invalid number of arguments #1171)
  • Refactored into a single clean main function with return values (Instead of using await exit() or invoking process.exit() directly, use return values #1194)
  • Better usage of output utilities
  • Fixes > color in > Error! output
  • Downgrades ava to 0.x so that it doesn't bring babel 7
  • now.js refactored to use exit utility
  • EPIPE fixes now apply to the entire codebase
  • Source maps support during development
  • Gets rid of broken 'sorting by current app in cwd' (only worked for package.json)
  • Makes now domains ls also render age instead of created for consistency
  • now ls only shows the latest deployment per app now. now ls [app] shows all
  • now ls --all is much faster by skipping trying to get instances when instanceCount = 0 (the field was introduced by v3 of list API)
  • Bunch of other little fixes

@leo leo added the 💅 patch label Mar 17, 2018
@rauchg
Copy link
Member Author

rauchg commented Mar 17, 2018

With the help of @timneutkens, I also added source map support.

Before:

> Error! An unexpected error occurred in ls: TypeError: Cannot read property 'length' of undefined
    at dotindex (/Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:59:32)
    at /Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:11:21
    at Array.forEach (<anonymous>)
    at forEach (/Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:73:31)
    at /Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:10:9
    at Array.reduce (<anonymous>)
    at reduce (/Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:63:30)
    at module.exports (/Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:9:20)
    at Object.main (/Users/rauchg/Projects/now-cli/dist/now.js:7060:15)
    at <anonymous>

after:

> Error! An unexpected error occurred in ls: TypeError: Cannot read property 'length' of undefined
    at dotindex (/Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:59:32)
    at /Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:11:21
    at Array.forEach (<anonymous>)
    at forEach (/Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:73:31)
    at /Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:10:9
    at Array.reduce (<anonymous>)
    at reduce (/Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:63:30)
    at module.exports (/Users/rauchg/Projects/now-cli/node_modules/text-table/index.js:9:20)
    at Object.main (/Users/rauchg/Projects/now-cli/src/providers/sh/commands/list.js:153:1)
    at <anonymous>

(notice the before-last lines in each stack trace)

@rauchg
Copy link
Member Author

rauchg commented Mar 18, 2018

This can be merged once we polish list v3. It currently is missing state and instanceCount for some deployments.

@@ -7,11 +7,15 @@ module.exports = {
entry: './src/now.js',
target: 'node',
externals: [nodeExternals()],
devtool: 'source-map',
Copy link
Contributor

Choose a reason for hiding this comment

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

@rauchg, just curious (as I was enabling source map support myself for fixing #1010): doesn't this devtoolsetting mean that source maps are always generated, even in production? Looking at https://webpack.js.org/configuration/devtool/ this setting is ok for production, but in your commit message you wrote something about (small) performance hits and that you only want to enable source maps in development.

Copy link
Contributor

Choose a reason for hiding this comment

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

My approach was around something like:

const isDev = process.env.NODE_ENV === 'development';

module.exports = {
  entry: './src/now.js',
  devtool: isDev ? 'source-map' : false,
// [...]

(... or introducing https://github.com/kentcdodds/webpack-config-utils if more prod/dev differences within the webpack config pop up.)

@riddla
Copy link
Contributor

riddla commented Mar 18, 2018

@rauchg (or @leo): Is it safe to base my fix for #1010 on this branch?

@leo leo changed the title [WIP] Improved now ls Improved the now ls command Mar 21, 2018
@leo
Copy link
Contributor

leo commented Apr 3, 2018

Closing in favor of #1205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants