-
-
Notifications
You must be signed in to change notification settings - Fork 377
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
Refactor: Move CLI create
into its own package
#1440
Conversation
|
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.
There's also a fair few stylistic changes done automatically by my IDE (and I didn't notice until now). I figure I'll keep them in, they look to be right. I can remove them if they make this too busy.
@@ -1,6 +1,7 @@ | |||
const { join } = require('path'); | |||
const { existsSync, unlinkSync, symlinkSync } = require('fs'); | |||
const cmd = require('../../lib/commands'); | |||
const create = require('../../../create-preact-app/lib/commands').create; |
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 is a bit messy, not sure if there's a better way
let prog = sade('create-preact-app').version(pkg.version); | ||
|
||
prog | ||
.command('create [template] [dest]', '', { default: true }) |
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 made this the default so it can be ran as npx create-preact-app default my-application
. I figure that's better than the alternative npx create-preact-app create default my-application
.
const { blue, yellow, red } = require('kleur'); | ||
|
||
const main = { | ||
info: blue('ℹ'), | ||
warning: yellow('⚠'), | ||
error: red('✖'), | ||
}; | ||
|
||
const win = { | ||
info: blue('i'), | ||
warning: yellow('‼'), | ||
error: red('×'), | ||
}; | ||
|
||
module.exports = process.platform === 'win32' ? win : main; |
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.
Items like this (that are duplicated) might need suggestions from someone who's more experienced with mono-repos. Not sure if there's a better solution without introducing another package that holds some of this common stuff?
|
||
// it('should fail given an invalid name', async () => { | ||
// const exit = jest.spyOn(process, 'exit'); | ||
// await create('default', '*()@!#!$-invalid-name'); | ||
// expect(exit).toHaveBeenCalledWith(1); | ||
// }); |
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.
Removed as this has been commented out and unused for 3 years: fef996e
create
into its own package
Personally preferred having it in the same package, it was separate enough to me, but I'm fine with it if other people agree |
It certainly adds some complexity to the repo and probably some slight duplication of code too. Just an idea for a lighter (and therefore faster with |
Closing this as I'd like to redo it. |
What kind of change does this PR introduce?
Refactor
Did you add tests for your changes?
No new tests, but old ones were updated/fixed
Summary
Currently the CLI performs two, mostly separate functions: creating new projects and running existing ones. With this combined functionality, the package is heavier than need be in both applications of the package. Especially now that
npx
is the suggested way to use the CLI to bootstrap new projects, having a lighter tool is very beneficial.This PR would separate out the
create
functionality of the CLI into what I've temporarily calledcreate-preact-app
. I do believe that name is taken, so will likely need a different name.This was mentioned in a comment on the change to use
npx
as a potential avenue for change in the future: #1355 (comment)Does this PR introduce a breaking change?
I don't think this counts as breaking?