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

New feature (compilation) status window #1032

Closed
wants to merge 25 commits into from

Conversation

Wernerson
Copy link
Contributor

What kind of change does this PR introduce?
feature

Did you add or update the examples/?
yes

Summary

When Webpack is compiling a little window appears and shows the compilation status (like building modules, optimizing chunks, etc.) and a progress bar indicating the progress (who would have thunk). The title of the browser tab also displays the progress in percent.

With this feature, you don't have to check your Terminal for the compilation status anymore. You see any important information right on the screen.

First idea in issue: #974

Does this PR introduce a breaking change?
If the webpack.config.js isn't modified there shouldn't be any breaking changes (as far as I tested) for existing projects.

Other information

  • Tested in a real-life project (thou without the changing title feature, going to test this tomorrow).
  • added example
  • added status option to optionsSchema.json (Validation test for this runs as well)

@jsf-clabot
Copy link

jsf-clabot commented Aug 9, 2017

CLA assistant check
All committers have signed the CLA.

@shellscape
Copy link
Contributor

Thanks for the PR! Could you provide a screenshot of what this feature looks like? We'll also need you to sign the CLA before we can move forward.

@Wernerson
Copy link
Contributor Author

I have tested the code in our real-life app. Worked as well.

Here is the screenshot (the window appears in the middle of the screen):
screenshot

@Wernerson
Copy link
Contributor Author

I resolved the conflicts but couldn't test them so far (can't update webpack at my workplace...).

@codecov
Copy link

codecov bot commented Aug 10, 2017

Codecov Report

Merging #1032 into master will increase coverage by 0.16%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1032      +/-   ##
==========================================
+ Coverage   72.25%   72.42%   +0.16%     
==========================================
  Files           4        4              
  Lines         465      475      +10     
  Branches      139      141       +2     
==========================================
+ Hits          336      344       +8     
- Misses        129      131       +2
Impacted Files Coverage Δ
lib/Server.js 79.82% <80%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b4729f...2711acb. Read the comment docs.

@shellscape
Copy link
Contributor

I resolved the conflicts but couldn't test them so far (can't update webpack at my workplace...).

That's an interesting restriction they have on you. Please let us know when you're able to test and confirm your conflict resolutions were correct.

@Wernerson
Copy link
Contributor Author

Found an error from the merge. The code is tested and working. Ready to merge from my side :)

@shellscape
Copy link
Contributor

Thanks for staying on top of this. We're taking a look at how best to move forward with this type of addition and feature on the client. We haven't forgotten about you :)

@Wernerson
Copy link
Contributor Author

Just had an idea. Maybe add an option to display the webpack console output also in the browser console? We could do this the same way with the overlay option. Either true (just displaying the status window) or an object with window and console properties... What do you think?

@shellscape
Copy link
Contributor

@Wernerson that's a good idea. we've been wrestling with how to handle additional features for the client like this - we've got the error overlay and now the proposal for the progress overlay. their styles are very different, but we don't have a unified guide for that. now it's clear that we may have other additions that folks may want to add to the client as well, so it presents an interesting question as to how to handle contributions for those moving forward. and your PR here was the catalyst for that 👍

all that being said, I don't think merging this PR right this moment is the right call. I do think that sending the progress info to the console is the right call right now, as it gives the user some valuable feedback immediately. however, I'd like to leave this PR open and in a pending state. that's because...

...what I'd like to do in the near future is to provide a mechanism for overlay plugins that contain their own business logic, move the warning and errors overlay into their own plugins, get this progress overlay into a plugin (I'd be happy to help with that), unify their styles (to match https://webpack.js.org/ as a guide), and release. this would ultimately open the door for any number of overlays on demand that won't need any kind of approval. it should also slim down the codebase for webpack-dev-server and overall be a win.

@Wernerson
Copy link
Contributor Author

@shellscape yeah that makes sense... I'm gonna think of a possible solution as well and will rewrite the progress feature as a plugin as soon as it's possible. Maybe we can close the PR and sort of label it? And maybe make an issue so we could gather opinions or possible solutions from others? 😄

@shellscape
Copy link
Contributor

shellscape commented Aug 29, 2017

@Wernerson we'll close this one, but please do take this branch and create a feature branch from it that adds the status to console messages as you suggested (but leaves out the overlay). I think that's a feature we can introduce immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants