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

add system info cli command #329

Merged
merged 7 commits into from Mar 25, 2020
Merged

add system info cli command #329

merged 7 commits into from Mar 25, 2020

Conversation

thedavidprice
Copy link
Contributor

Notes/Questions:

  • ESlint warning for 'envinfo' import: anything I should/can do about this?
  • open to suggestions about option name/alias of "clipboard" and "C"
  • automatic "copy to clipboard" is nice, but clipboardy is a beast of an import and I'm mixed on whether this is necessary
  • re: Info Output
    • many references include output for 'System: CPU' -- will we need this?
    • I added 'Databases: SQLite' -- since this is handled by Prisma2, is it helpful here?

@thedavidprice
Copy link
Contributor Author

Closes #321

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

This is awesome! I've left some feedback that might make the code easier to read and a bit more modern.

packages/cli/src/commands/info.js Outdated Show resolved Hide resolved
npmPackages: ['@redwoodjs/core'],
Databases: ['SQLite'],
})
.then((envinfoOutput) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think our handlers support async/ await. So you could write this as:

export const handler = async () => {

   try {
     const output = await envinfo.run()
   } catch (e) {
     console.error("Cannot access env info.")
     process.exit(1) 
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use async/await. The const output = ... didn't seem necessary here. But I could be missing something obvious; just let me know.

packages/cli/src/commands/info.js Outdated Show resolved Hide resolved
@peterp
Copy link
Contributor

peterp commented Mar 24, 2020

ESlint warning for 'envinfo' import: anything I should/can do about this?

Can you let me know what the import error is?

automatic "copy to clipboard" is nice, but clipboardy is a beast of an import and I'm mixed on whether this is necessary

I personally don't think we should mess with people's clipboards.

re: Info Output

I think what you have is perfect.

@satyarohith
Copy link
Contributor

satyarohith commented Mar 24, 2020

I personally don't think we should mess with people's clipboards.

The output is only copied when a user deliberately passes the flag, right? I think it's handy.

@weaversam8
Copy link

I think this is in great shape!

automatic "copy to clipboard" is nice, but clipboardy is a beast of an import and I'm mixed on whether this is necessary

Though it's a nice feature, I lean towards the idea of keeping things lean. It isn't that difficult to copy something to clipboard manually.

re: Info Output

Looks great! Do we want to add printing out the version of prisma2 or @prisma/client? I know we're supposed to be pinned on an internal version, but just in case somebody tried to install their own in the future, might be helpful just to verify we're running the same version.

@thedavidprice
Copy link
Contributor Author

thedavidprice commented Mar 24, 2020

Thanks, all. I’ll get back to these comments later today.

Realized I never added the current output for feedback:

$ yarn rw info -C                   
yarn run v1.22.4
$ /Users/price/Repos/xx-redwoodblog/node_modules/.bin/rw info -C

  System:
    OS: macOS 10.15.3
    Shell: 5.7.1 - /bin/zsh
  Binaries:
    Node: 12.16.0 - /var/folders/gj/_y54h7q11mz9rwnhn16h6yrr0000gn/T/yarn--1585031216782-0.8567735743019371/node
    Yarn: 1.22.4 - /var/folders/gj/_y54h7q11mz9rwnhn16h6yrr0000gn/T/yarn--1585031216782-0.8567735743019371/yarn
  Databases:
    SQLite: 3.28.0 - /usr/bin/sqlite3
  Browsers:
    Chrome: 80.0.3987.149
    Firefox: 73.0.1
    Safari: 13.0.5
  npmPackages:
    @redwoodjs/core: ^0.2.2 => 0.2.5 

System info copied to clipboard ✂️ 📋

✨  Done in 1.13s.

@thedavidprice
Copy link
Contributor Author

Thanks for the feedback and help here, everyone. Latest version changes:

  • cleaned up the code ('cause I did a late-night copy/paste and it needed some love!)
  • I vetoed the "automatic copy" myself. It seems unnecessary in practice. And we'll find out either way if it's needed based on future use. Easy to add now that we have an example in the branch.

@peterp
Import ESLint Warning
We have this occurring elsewhere and I believe it more about the modules TS support more than anything else. So not actionable. But for fyi:

Could not find a declaration file for module 'envinfo'. '/Users/price/Repos/redwoodjs-redwood/node_modules/envinfo/dist/envinfo.js' implicitly has an 'any' type.
  Try `npm install @types/envinfo` if it exists or add a new declaration (.d.ts) file containing `declare module 'envinfo';`ts(7016)

Copy link

@weaversam8 weaversam8 left a comment

Choose a reason for hiding this comment

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

nice work! One last thing - you left the "copy-to-clipboard" flag included on the command, even though we took out that functionality.

(whoops, forgot to include suggested changes, sorry, you can delete this comment)

yarn.lock Outdated Show resolved Hide resolved
Copy link

@weaversam8 weaversam8 left a comment

Choose a reason for hiding this comment

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

@thedavidprice - nice work! One last thing - you left the "copy-to-clipboard" flag included on the command, even though we took out that functionality.

@thedavidprice
Copy link
Contributor Author

Note: some updates to our ESLint/Prettier config were causing Lint check error. I committed simple fix to master here bd98d07

@thedavidprice thedavidprice merged commit e5c0a16 into master Mar 25, 2020
@thedavidprice thedavidprice deleted the dsp-add-command-info branch March 25, 2020 20:27
mohsen1 pushed a commit to mohsen1/redwood that referenced this pull request Apr 12, 2020
@thedavidprice thedavidprice added this to the unassigned-version milestone May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants