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: Add init command #1358
feat: Add init command #1358
Conversation
bin, | ||
commands: './dist/commands', | ||
dirname: bin, | ||
...packageJSON.oclif, |
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.
I have this at the bottom to override anything that the init command is doing. This seems like the right thing to do in an existing project.
static summary = 'Initialize a new oclif CLI' | ||
|
||
async run(): Promise<void> { | ||
const outputDir = await this.getFlagOrPrompt({defaultValue: './', name: 'output-dir', type: 'input'}) |
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.
I went back and forth on whether this should be a flag or an arg. I landed on flag to better match how generate
works but I don't feel strongly either way.
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.
Do you think an output-dir flag or arg is really helpful in this scenario? Wondering if you should just drop it altogether
If you keep it, I like it as a flag
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.
I think it's good to allow tools like this to operate in other directories, like npm --prefix /path/to/project
or git -C /path/to/project
|
||
let moduleType = this.flags['module-type'] | ||
if (!moduleType || !validModuleTypes.includes(moduleType)) { | ||
if (packageJSON.type === 'module') { |
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.
I think this is about as good as we can do with module auto-detection here. We could parse tsconfig.json
(module seemed to be the strongest signal) but I 'm not sure we want to go that far with something that doesn't require TS.
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.
I think checking type
in package.json is good enough. You could also use get-package-type which we use in @oclif/core. The only benefit to that package is that it caches the result. But probably not worth in this scenario since we're only ever going to check once
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.
Looking good! Just a few questions/changes
I'll checkout this out later today to test it out
|
||
let moduleType = this.flags['module-type'] | ||
if (!moduleType || !validModuleTypes.includes(moduleType)) { | ||
if (packageJSON.type === 'module') { |
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.
I think checking type
in package.json is good enough. You could also use get-package-type which we use in @oclif/core. The only benefit to that package is that it caches the result. But probably not worth in this scenario since we're only ever going to check once
src/commands/init.ts
Outdated
|
||
const template = moduleType === 'ESM' ? 'hello-world-esm' : 'hello-world' | ||
const repoPath = join(location, '_tmp-template') | ||
await clone(template, repoPath) |
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.
What are your thoughts on adding bin/run.js
and bin/dev.js
templates to the /templates
directory and using those instead?
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.
I think that would be much better than cloning a repo and hoping the permissions and everything works.
Would that make maintenance on your template repos more arduous?
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.
Yeah that would be the downside but we already have to maintain the other templates to keep up with the hello-world templates so it's not a new problem.
I think I'd prefer templates for now. We can always solve that problem later
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.
@mdonnalley - Just to be clear ... are you saying you want me to use templates added to this repo for this PR?
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.
Yes, I would prefer for you to add templates for all the bin scripts to the templates/
directory.
You'll just need to insert __dirname
or import.meta.url
based on the module type
src/commands/init.ts
Outdated
} | ||
} | ||
|
||
await exec(`${packageManager} install @oclif/core`, {cwd: location, silent: false}) |
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.
yarn install @oclif/core
needs to be yarn add @oclif/core
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.
Good catch, I haven't used yarn in a while ... looks like that's the same in pnpm
static summary = 'Initialize a new oclif CLI' | ||
|
||
async run(): Promise<void> { | ||
const outputDir = await this.getFlagOrPrompt({defaultValue: './', name: 'output-dir', type: 'input'}) |
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.
Do you think an output-dir flag or arg is really helpful in this scenario? Wondering if you should just drop it altogether
If you keep it, I like it as a flag
@mdonnalley - Alright, we should be good here 👍 |
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 looks good to me - thanks again for the contribution!
I'm going to merge this into a separate branch before merging to main so I can add some integration tests before it's released.
Depending on what the tests reveal, I might make some modifications but your initial commit will still be in the history so you'll get attributed with the bulk of the work. Hopefully that works for you
* feat: add init command (#1358) * feat: add init command * refactor: validation for module types and package managers * feat: add default comman discovery * feat: allow existing oclif config to override new * feat: keep existing bin entries * feat: command description and examples * fix: remove colons from prompts * fix: default bin value to empty string if dir not found * fix: path permissions to clone repo and ensure bin dir exists * fix: mkdir recursive to ensure bin directory exists * fix: update package command in init * feat: switch repo cloning to ejs template * feat: tests and ux improvements * fix: more ux improvements * fix: return type on getFlagOrPrompt * chore: apply suggestions from code review Co-authored-by: Josh Cunningham <josh@joshcanhelp.com> * feat: add back module-type flag --------- Co-authored-by: Josh Cunningham <josh@joshcanhelp.com>
This adds a new command,
init
, that adds the necessary files and configuration to an existing npm project. It attempts to auto-detect the module type and package manager being used, falling back to a prompt if necessary. Usage is covered in the examples.Addresses #999