-
Notifications
You must be signed in to change notification settings - Fork 44
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
Rewrite CLI to use mixins for logging purposes #632
Conversation
7fea2f6
to
2f19f7a
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.
LGTM!
603b562
to
e2b15a6
Compare
bccb06e
to
56da848
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.
LGTM 👍
} | ||
} | ||
|
||
module.exports = class CLIMixin { |
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.
We could add extends Mixin
here to make it clear(er) it's a mixin.
That would also be good in order to not break as soon as it is actually required to inherit from Mixin.
And thats what we are doing anywhere else.
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 don't think we can - at least not while the mixin is still part of @untool/core
, because then the CLI (which might be installed globally) will have a dependency to @untool/core
.
But what we could do, is to make the Mixin
a separate package in untool and then we could add it (or rather hops-mixin
) as a dependency to the CLI package.
/cc @dmbch
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.
As the Mixin
base class is just a five-liner, I am opposed to releasing it separately. I personally think that for this very use-case, it is completely fine to have a standalone class.
56da848
to
3eb7286
Compare
Co-authored-by: dmbch <daniel@dmbch.net>
3eb7286
to
7b18878
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.
LGTM
At the moment the CLI does not use a mixin and log output for build and
server is done in the hops-express and hops-webpack package.
Inspired by the CLI package in untool, we now use a CLI mixin which
takes care of logging for express and webpack.
TODO: With this PR the webpack readme will be removed - we shouldmake sure that the necessary information is retained someplace else.
Edit: The express and webpack readmes are now part of: #655