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

feat: no yeoman #1321

Merged
merged 15 commits into from Mar 19, 2024
Merged

feat: no yeoman #1321

merged 15 commits into from Mar 19, 2024

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Mar 6, 2024

@W-15044742@
@W-15161957@

@jpshackelford
Copy link

jpshackelford commented Mar 8, 2024

Very glad to see this work. I am making a choice between oclif and commander today and seeing these warnings had me worried about whether oclif was actively maintained. I certainly would not want users of my CLI to be greeted with this. Thanks for the PR!

$ npm install -g oclif 
npm WARN deprecated debuglog@1.0.1: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
npm WARN deprecated readdir-scoped-modules@1.1.0: This functionality has been moved to @npmcli/fs
npm WARN deprecated @npmcli/move-file@1.1.2: This functionality has been moved to @npmcli/fs
npm WARN deprecated @npmcli/move-file@2.0.1: This functionality has been moved to @npmcli/fs
npm WARN deprecated @npmcli/move-file@2.0.1: This functionality has been moved to @npmcli/fs

added 1052 packages in 13s

@moltar
Copy link
Contributor

moltar commented Mar 14, 2024

Can't wait for this to merge. Such an eye sore in the terminal to see all those warnings.

@mdonnalley
Copy link
Contributor Author

Thanks @moltar - hoping to get this out tomorrow or early next week. Just waiting on a coworker to review and test.

const {login, name} = await got('https://api.github.com/user', {headers}).json<{login: string; name: string}>()
return {login, name}
} catch {}
}
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find where yeoman was using this env, seems it did a while ago but now it does an unauthenticated request, and relies on the user having an email setup in git:

https://github.com/yeoman/generator/blob/71dcba00427d7e382c1c32af68dbb7666ee0269f/src/actions/user.ts#L51-L52

https://github.com/sindresorhus/github-username/blob/c716cf8e920430dc319c44ab7586ab2e72b91467/index.js#L18

Either way works so it should be fine, the precedence seems for getting the username looks like:

  1. GH API (if token)
  2. git username (if available)
  3. pjson.author in template

I'll check on QA 👍🏼

cristiand391
cristiand391 previously approved these changes Mar 18, 2024
Copy link
Member

@cristiand391 cristiand391 left a comment

Choose a reason for hiding this comment

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

lgtm. I'll QA mainly on windows to ensure the generator works there.

@cristiand391
Copy link
Member

QA notes:

✅ can generate a CLI/command/hook successfully on windows, all paths are correct and can run command/hooks/tests
✅ values provided via flags are respected and only get prompted for the missing ones
✅ both npm/yarn work for generated plugins
NOTE: now that the default PM is npm the first npm install takes a bit longer because there's no package-lock.json in the templates repo, so users might get something different from what gets tested in the templates repo.

✅ all defaults with --all
❌ can't make it pick up my username from local git

latest oclif:
Screenshot 2024-03-18 at 18 20 26

this branch:
Screenshot 2024-03-18 at 18 20 52

➜  oclif git:(mdonnalley/no-yeoman) ✗ git config --get user.name
Cristian Dominguez

so the homepage/bug fields are wrong in the pjson:

...
  "homepage": "https://github.com/oclif/qa2",
  "bugs": "https://github.com/oclif/qa2/issues",
...
`

@cristiand391
Copy link
Member

qa update:

✅ get username from git:

➜  oclif git:(mdonnalley/no-yeoman) ./bin/run.js generate asdf
Generating asdf in /Users/cdominguez/code/gh/oclif/oclif/asdf
? Select a module type ESM
? NPM package name asdf
? Command bin name the CLI will export asdf
? Description oclif example Hello World CLI (ESM)
? Author Cristian Dominguez
? License MIT
? Who is the GitHub owner of repository (https://github.com/OWNER/repo) oclif
? What is the GitHub name of repository (https://github.com/owner/REPO) asdf
? Select a package manager (Use arrow keys)
❯ npm
  yarn

NOTE
yeoman was able to get the GH username from the email found in git config using the github-username pkg, so the default value for owner would be that, but now you need to manually type it for the URLs to be right.

➜  oclif git:(mdonnalley/no-yeoman) ✗ oclif generate qa2
Time to build an oclif CLI! Version: 4.5.7
? Select a module type CommonJS
Cloning into '/Users/cdominguez/code/gh/oclif/oclif/qa2'...
? npm package name qa2
? command bin name the CLI will export qa2
? description oclif example Hello World CLI
? author Cristian Dominguez @cristiand391
? license MIT
? Who is the GitHub owner of repository (https://github.com/OWNER/repo) cristiand391
? What is the GitHub name of repository (https://github.com/owner/REPO) qa2
? Select a package manager (Use arrow keys)
  npm
❯ yarn

@cristiand391 cristiand391 merged commit 30a8a53 into main Mar 19, 2024
22 checks passed
@cristiand391 cristiand391 deleted the mdonnalley/no-yeoman branch March 19, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants