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

[Feature]: Abstract progressplugin to webpack contrib #9252

Closed
evenstensberg opened this issue Jun 11, 2019 · 18 comments
Closed

[Feature]: Abstract progressplugin to webpack contrib #9252

evenstensberg opened this issue Jun 11, 2019 · 18 comments
Labels

Comments

@evenstensberg
Copy link
Member

Feature request

https://github.com/webpack/webpack/blob/master/lib/ProgressPlugin.js

It would be nice to abstract the progressplugin to webpack contrib if we can do that to avoid code that some users don't need as apart of webpack. Furthermore I think we should allow the progressplugin to have more flavors instead of one bar that is outputting in a single fashion.

I suggest that we can make use of Ora as well as look out for other spinners that might make the CLI visually better and improve the user experience for webpack. I'm up for doing this task if allowed.

Even

What is the expected behavior?
ProgressPlugin with various of flavors

What is motivation or use case for adding/changing the behavior?
Better Interface

How should this be implemented in your opinion?
Abstracted Progress Plugin to another repository with more options

Are you willing to work on this yourself?
yes

@evenstensberg
Copy link
Member Author

CC @webpack/contributor-team for insight

@evenstensberg
Copy link
Member Author

Relevant: webpack/webpack-cli#717

@alexander-akait
Copy link
Member

Don't think it is should be in webpack-contrib org, better use webpack org for this.

Why don't use this?

new ProgressPlugin({ handler: customHandle });

@hulkish
Copy link
Contributor

hulkish commented Jun 11, 2019

@evenstensberg hmm, i agree with the decoupling of progress plugin. but, i personally view the ora usage a little undesirable from a dependency standpoint

@evenstensberg
Copy link
Member Author

We can do that. Does it make sense to move it as a seperate plugin in /webpack ?

@okonet
Copy link

okonet commented Jun 12, 2019

@evenstensberg I’m still getting email notifications for webpack issues.

You are receiving this because you are on a team that was mentioned.

Can you please check and remove me from those teams? Thanks!

@alexander-akait
Copy link
Member

@evenstensberg

We can do that. Does it make sense to move it as a seperate plugin in /webpack ?

Will be great to look on PoC, it is hard to say how to solve problem(s) better without examples. We already have option to change behavior as i written above. Why it is not solve problem?

@sokra
Copy link
Member

sokra commented Jun 13, 2019

I think it's difficult to move the ProgressPlugin into a separate repo. It's very coupled to webpack internals so keeping it up-to-date as separate repo would be a hard job.

You can still build a ora progress plugin on top of this like @evilebottnawi mentioned: #9252 (comment)

So I don't see strong arguments for separating it.

@evenstensberg
Copy link
Member Author

Gotcha

@niieani
Copy link
Contributor

niieani commented Jun 13, 2019

I think what's more important is to make the handler payload better. There's different types of progress that we can track and we should have separate object signatures for each, rather than just sending a bunch of pre-prepared strings down to it.

Currently in order to make a good handler you need to parse this information our with something like RegExps, which is really bad for performance and generally a poor experience.

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 these parts properly. The result is something like this:

progress-bar

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.

The default handler would simply makes these strings as they are now, so there wouldn't be a "breaking change" in the way we display it - we get the same behavior, and still give greater flexibility for alternative handlers.

It could also be a disjoint union of various statuses with their own meta, ideally typed with JSDoc.

@alexander-akait
Copy link
Member

@niieani do you have idea how we can change plugin for better integration as you written above? Maybe you can send PoC?

@evenstensberg
Copy link
Member Author

Reopening for design discussion

@evenstensberg evenstensberg reopened this Jun 13, 2019
@niieani
Copy link
Contributor

niieani commented Jun 13, 2019

I don't really have time for a POC these days, but here's the part I'm talking about:

const update = () => {
const percentByModules =
doneModules / Math.max(lastModulesCount, moduleCount);
const percentByEntries =
doneEntries / Math.max(lastEntriesCount, entriesCount);
const items = [
0.1 + Math.max(percentByModules, percentByEntries) * 0.6,
"building"
];
if (showEntries) {
items.push(`${doneEntries}/${entriesCount} entries`);
}
if (showModules) {
items.push(`${doneModules}/${moduleCount} modules`);
}
if (showActiveModules) {
items.push(`${activeModules.size} active`);
items.push(lastActiveModule);
}
handler(...items);
};

You can see here we're pre-preparing some ways to display this data, e.g.:

items.push(`${doneEntries}/${entriesCount} entries`)

and then feeding all that to the handler in "random" order:

handler(...items)

I say random, because it's not predictable - depending on the settings and stage of compilation you'll get different strings (items) pushed to the handler.

My suggestion is to simply provide the raw metadata, like:

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

and then leave it up to the handler to format this data in whatever way it wants.

@alexander-akait
Copy link
Member

/cc @sokra what do you think?

@sokra
Copy link
Member

sokra commented Jun 14, 2019

@niieani Makes sense to make it more customizable.

Maybe it makes even sense to split the jobs into handler and formater, so it's called in this way:

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

This could keep compatiblity with existing handlers, makes it easy for the simple case, but allows customizablility for the advanced case. It would also allows to only change the format while keeping the default handler (which prints to stderr)

@webpack-bot
Copy link
Contributor

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@alexander-akait
Copy link
Member

bump, still feel free to sens a PR with PoC

@webpack-bot
Copy link
Contributor

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

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

No branches or pull requests

7 participants