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: add init command (#1358) #1366

Merged
merged 6 commits into from Apr 10, 2024
Merged

feat: add init command (#1358) #1366

merged 6 commits into from Apr 10, 2024

Conversation

mdonnalley
Copy link
Contributor

Merges #1358

Fixes #999

* 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
@mdonnalley
Copy link
Contributor Author

@joshcanhelp FYI I've made a few changes. Feel free to push back on any of them

  • added integration tests to ensure this works across package managers, operating systems, and module types
  • made --output-dir a normal flag instead of a promptable flag. I think it'll be a better UX for oclif init to simply default to the current working directory without prompting
  • removed the --module-type flag. I think it's safe to work off the type in the package.json and I can't imagine a reasonable scenario where someone with a CommonJS project would want to init oclif with the ESM bin scripts (or vice versa)
  • added a --topic-separator promptable flag. I think it'll be nice for people to have that option set for them, especially since oclif defaults to colons - which few people actually want.
  • run chmod on the bin scripts so that they're runnable
  • install ts-node if it's not already there since it's needed for bin/dev.js
  • a few code preference/quality changes

Again, feel free to push back on any of these changes

@joshcanhelp
Copy link
Contributor

Everything makes sense to me except:

removed the --module-type flag. I think it's safe to work off the type in the package.json and I can't imagine a reasonable scenario where someone with a CommonJS project would want to init oclif with the ESM bin scripts (or vice versa)

My only hesitation here is that you can use .mjs and .cjs extensions to indicate the module type that you want to use without type being in the package.json (v18 docs and v20 docs and others going back).

src/commands/init.ts Outdated Show resolved Hide resolved
src/commands/init.ts Outdated Show resolved Hide resolved
src/commands/init.ts Outdated Show resolved Hide resolved
Co-authored-by: Josh Cunningham <josh@joshcanhelp.com>
@mdonnalley
Copy link
Contributor Author

My only hesitation here is that you can use .mjs and .cjs extensions to indicate the module type that you want to use without type being in the package.json (v18 docs and v20 docs and others going back).

That's a good point that I hadn't considered. I'll add it back 👍

@mdonnalley mdonnalley merged commit c1ece19 into main Apr 10, 2024
54 checks passed
@mdonnalley mdonnalley deleted the mdonnalley/1358 branch April 10, 2024 15:04
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.

Add oclif to existing project
2 participants