-
Notifications
You must be signed in to change notification settings - Fork 352
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
fix: remove oclif framework and fixes an issue with sites:create #3717
Conversation
📊 Benchmark resultsComparing with f196c10 Package size: 354 MB⬇️ 2.56% decrease vs. f196c10
|
f96c102
to
f6b4e09
Compare
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.
Hi @lukasholzer, this is great work.
I left some comments and keep doing so, but I'll let you go over the existing ones while I continue the review to unblock you.
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 good to go @lukasholzer 🚀
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes https://github.com/netlify/pod-workflow/issues/343
Fixes #3716
Good to know when reviewing
.option('--data [data]')
brackets make data optional can be undefined as well.option('--data <data>')
angle brackets make passing a data required.Refactorings:
create...Command
function that has one parameter theprogram
and returns one.index.js
) that exports thecreate....Command
function of the main command (not the sub ones) and if needed the action (like in theinit
case where the init command is reused).src/commands/base-command.js
and every command extends from it.src/lib/fs.js
methods likereadFileAsync
throughfs.promises.readFile
as there is no need to promisify them.Commands to refactor:
Other things to do
error
,warning
andexit
things// TODO: ....