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

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);
Copy link
Member

Choose a reason for hiding this comment

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

rename to buildEntry and also pass name as arguments

Copy link
Member Author

@smelukov smelukov Oct 29, 2018

Choose a reason for hiding this comment

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

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}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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

@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

The minimum test ratio has been reached. Thanks!

@smelukov
Copy link
Member Author

smelukov commented Nov 2, 2018

@sokra any comments here? 😊

@sokra
Copy link
Member

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

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
@sokra
Copy link
Member

sokra commented Nov 5, 2018

Thanks

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

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 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
Development

Successfully merging this pull request may close these issues.

Custom logic for ProgressPlugin percentage calculation
4 participants