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

Better output #1196

Merged
merged 5 commits into from
Feb 3, 2020
Merged

Better output #1196

merged 5 commits into from
Feb 3, 2020

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Jan 30, 2020

What kind of change does this PR introduce?
Work in progress of #717

This PR introduces the following changes:

  • compiler.js => Compiler.js as class. I holds all the same methods as before. It also holds a class CompilerOutput.js which is responsible to generate the wanted output
  • CompilerOutput.js: this class is responsible to output based on certain options. At the moment I left the "fancy" compilation but I think this fancy way of showing the results should be "opt-in" by passing an argument. Or we should provide a way to switch back to the old output. I also prepared some other functions for emit the normal output and the JSON one.
  • I fixed eslint around some files. We were using a plugin that was not needed for the JS files.

Did you add tests for your changes?
Not yet, will do

If relevant, did you update the documentation?
No, I am awaiting feedback

Summary
The output must contain at least all the information emitted by the method stats.getOutput.
Hopefully I managed to put all of them but I need to try it with some other project.

Does this PR introduce a breaking change?

Nope

Other information
Screenshot 2020-01-30 at 15 42 20

Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico looks much more informative now :)
Just one more issue, cli-table seems to overflow in case of small terminal windows.
Maybe we can go with some other layout here?
image

@ematipico
Copy link
Contributor Author

@rishabh3112 I can remove the header's width so the table will be narrower:

Screenshot 2020-01-31 at 09 16 38

@ematipico
Copy link
Contributor Author

Honestly, I'd want to propose an argument called --output-pretty which will emit this output. And if we don't pass anything, we keep the standard webpack output. What do you think?

@rishabh3112
Copy link
Member

Honestly, I'd want to propose an argument called --output-pretty which will emit this output. And if we don't pass anything, we keep the standard webpack output. What do you think?

Yes, we should do this work this way.

I experimented with your work a little to make pretty output more compact.
image
image
WDYT?

@ematipico
Copy link
Contributor Author

I love it!!

@rishabh3112
Copy link
Member

@ematipico I added that change to your branch :)

@rishabh3112
Copy link
Member

rishabh3112 commented Jan 31, 2020

I thought and I think we can do other way around. Like add something like a --verbose flag for standard output and give this output by default.

@ematipico
Copy link
Contributor Author

--verbose is not the standard output. The standard output is always stripped/limited to a certain number of lines (it doesn't contain all the modules). So we should get a default, standard and verbose. We are release a major version, so we could use this as default and we need also a way to opt-in the standard one using another argument that is not --verbose

@rishabh3112
Copy link
Member

So what could be a possible flag name?
--output-standard?

@ematipico
Copy link
Contributor Author

Sounds good!

@ematipico ematipico changed the title [WIP] Better output Better output Feb 3, 2020
@ematipico ematipico marked this pull request as ready for review February 3, 2020 10:38
@ematipico ematipico requested a review from a team as a code owner February 3, 2020 10:38
@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@ematipico
Copy link
Contributor Author

@rishabh3112 I'd say we can make a different PR to add that new flag

Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will take a try at it.
This looks good to me.

@rishabh3112 rishabh3112 merged commit d72f9f8 into webpack:next Feb 3, 2020
@rishabh3112
Copy link
Member

Thanks and Cheers!

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.

None yet

3 participants