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

ProgressPlugin: support progress by entry points #8279

Merged
merged 10 commits into from Nov 5, 2018

Conversation

@smelukov
Copy link
Member

@smelukov smelukov commented Oct 25, 2018

closes #8088

What kind of change does this PR introduce?

  • support progress by entries (by mode options)
  • configuring of magic number (only for progress by modules)
  • scheme for plugin options
  • global default plugin options

Did you add tests for your changes?

I have no idea which tests can test a building progress...

Does this PR introduce a breaking change?

no

What needs to be documented once your changes are merged?

about new options


2018-10-26 01-09-25 2018-10-26 01_10_42

@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Oct 25, 2018

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)
lib/Compilation.js Outdated Show resolved Hide resolved
@@ -1041,6 +1050,8 @@ class Compilation extends Tapable {
* @returns {void} returns
*/
addEntry(context, entry, name, callback) {
this.hooks.addEntry.call(entry);

This comment has been minimized.

@sokra

sokra Oct 29, 2018
Member

rename to buildEntry and also pass name as arguments

This comment has been minimized.

@smelukov

smelukov Oct 29, 2018
Author Member

addEntry is not the same that buildEntry
addEntry will be called when adding an entry. This need to calculate total entries
buildEntry will be called when the module for an entry will be built
Or what do you mean? :)

lib/Compilation.js Outdated Show resolved Hide resolved
lib/Compilation.js Outdated Show resolved Hide resolved
lib/ProgressPlugin.js Outdated Show resolved Hide resolved
lib/ProgressPlugin.js Outdated Show resolved Hide resolved
schemas/plugins/ProgressPlugin.json Outdated Show resolved Hide resolved
schemas/plugins/ProgressPlugin.json Show resolved Hide resolved
`${doneModules}/${moduleCount} modules`,
0.1 + (doneModules / Math.max(lastCount, count)) * 0.6,
`building ${mode}`,
`${doneModules}/${count} ${mode}`,

This comment has been minimized.

@niieani

niieani Oct 29, 2018
Contributor

It would be much more useful if the handler took raw data (as an object or arguments in a particular order), rather than pre-made strings. E.g. in my implementation of a progress bar using the ProgressPlugin I have to resort to regexps/splits to re-parse the data out of the strings in order to be able to position them:

progress-bar

If instead we provided a default handler that makes these strings above, we can get the same behavior, and still give greater flexibility to consumers, alternative handlers.

I would imagine handler being most flexible e.g. when called like this:

handler({compilerIndex, currentStage, doneModules, activeModules, lastModulesCount, moduleCount, count, mode})

And some of those parameters would obviously be undefined in certain stages (which is fine). By stage I mean the hook currently being executed, or compilation.

This comment has been minimized.

@smelukov

smelukov Oct 29, 2018
Author Member

@niieani Seems like this is a breaking change, but in this PR I don't want to make any breaking changes
I think you may create your own PR for this feature to the next-branch

@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Oct 29, 2018

@smelukov Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Oct 29, 2018

The minimum test ratio has been reached. Thanks!

@smelukov
Copy link
Member Author

@smelukov smelukov commented Nov 2, 2018

@sokra any comments here? 😊

@sokra
Copy link
Member

@sokra sokra commented Nov 4, 2018

I'll finish it soon

@sokra sokra closed this Nov 5, 2018
@sokra sokra reopened this Nov 5, 2018
@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Nov 5, 2018

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

@sokra sokra merged commit 0293c3a into webpack:master Nov 5, 2018
12 of 14 checks passed
12 of 14 checks passed
@codecov
codecov/patch/integration 89.7% of diff hit (target 90%)
Details
@codecov
codecov/project/integration 91.62% (-8.38%) compared to 10ff7a5
Details
@codecov
codecov/changes/basic No unexpected coverage changes found.
Details
@codecov
codecov/changes/integration No unexpected coverage changes found.
Details
@codecov
codecov/changes/unit No unexpected coverage changes found.
Details
@codecov
codecov/patch/basic Coverage not affected when comparing 10ff7a5...e35d084
Details
@codecov
codecov/patch/unit Coverage not affected when comparing 10ff7a5...e35d084
Details
@codecov
codecov/project/basic 100% remains the same compared to 10ff7a5
Details
@codecov
codecov/project/unit 100% (target 0%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
security/snyk - package.json (sokra) No manifest changes detected
@azure-pipelines
webpack.webpack Build #20181105.5 succeeded
Details
@sokra
Copy link
Member

@sokra sokra commented Nov 5, 2018

Thanks

@smelukov smelukov deleted the smelukov:support-entry-progress branch Nov 5, 2018
@smelukov
Copy link
Member Author

@smelukov smelukov commented Nov 5, 2018

@sokra Seems like it's broken by this change
I've set entries to true and got 68% building 198/804 entries 2050/2115 modules 65 active .....
198/804 is not 68%
It should be Math.min - https://github.com/webpack/webpack/blob/master/lib/ProgressPlugin.js#L138

@smelukov
Copy link
Member Author

@smelukov smelukov commented Nov 5, 2018

Fixed in #8335

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

Successfully merging this pull request may close these issues.

4 participants