[WIP] refactor: next

[alexander-akait]

Note:

• update webpack-defaults
• Dropped webpack@3 support
• Dropped node@4 support
• Move all compression options (level and etc) in compressoinOptions
• cache: '/path/to/cache' is documented and tested
• union logic asset and filename option (tested and documented)

[michael-ciniawsky]

zopfli has license issues to my knowledge, so we can't use it at all as it seems 😥 Not sure about brotli, there is discussion in node nodejs/node#18964 to add it and we could/shoud support it then. I'm not really looking foward to support any third party packages tbh

[alexander-akait]

@michael-ciniawsky let's left as is, maybe we can add this option in future (if somebody request this)

[michael-ciniawsky]

• compressOptions or just compress as the option name ? (options.compress || options.compressOptions)
• Do we need deleteOriginalAssets, normally the uncompressed assets are used as fallback and shouldn't require to much disk space?
  • If we need it, could we rename it to e.g delete or originals etc instead ?

⚠️ I review it the PR again later, but the commits messages need to be updated so we can rebase merge this PR (for the CHANGELOG). Especially the commits containing BREAKING CHANGES need to have a BREAKING CHNAGE: <description> in the commit message body e.g

refactor(index): unify compress options

BREAKING CHANGE: `options.x` was renamed to `options.compressOptions.x`
BREAKING CHANGE: `options.y` was renamed to `options.compressOptions.y`
...

to

• Correctly major bump the version
• Contain a BREAKING CHANGES section in the CHANGELOG listing those

[alexander-akait]

@michael-ciniawsky

compressOptions or just compress as the option name ? (options.compress || options.compressOptions)

compress is misleading (compress it is a verb), better use compressOptions

Do we need deleteOriginalAssets, normally the uncompressed assets are used as fallback and shouldn't require to much disk space?

Yes we need this, some developers use this (especially on Heroku, you can setup what you server use only compressed assets and it is speed up, also you don't need original assets after this - less disk usage ). Better use this name for option, because delete and originals is misleading. The names of options should completely reflect what they do, do not try to cut their name, it is very bad practice.

I review it the PR again later, but the commits messages need to be updated so we can rebase merge this PR (for the CHANGELOG). Especially the commits containing BREAKING CHANGES need to have a BREAKING CHNAGE: in the commit message body e.g

I will write Changelog manually here.

Please review code asap, I do not like when people start to accumulate PR with messages I will review this in future. There is a chance that you will never have time for this, since the amount of work is always growing, if you do not have time for this, you can just trust another developer.

[michael-ciniawsky]

compress is misleading (compress it is a verb), better use compressOptions

I disagree with options.compressOptions as it is just redundant and verbose, if using a verb is the main concern here, what about compression instead of compress?

Better use this name for option, because delete and originals is misleading

What is exactly misleading? delete is not the best choice agreed, but originals: true || false should be straightfoward and less to type

I will write Changelog manually here.

Why?

Please review code asap, I do not like when people start to accumulate PR with messages I will review this in future. There is a chance that you will never have time for this, since the amount of work is always growing, if you do not have time for this, you can just trust another developer.

Yes I will review asap. Which other developer, there sadly is none to review and sign-off instead (which should definitely be required for things like this)?

[alexander-akait]

I disagree with options.compressOptions as it is just redundant and verbose, if using a verb is the main concern here, what about compression instead of compress?

Options should be verbose 😄 We always use *Options (example uglifyOptions) for third party libraries (terser/uglify/babel-minify and etc plugin). And we must not violate this agreement. Also, wasting time on the name of reducing the name of the option for a couple of characters is weird.

What is exactly misleading? delete is not the best choice agreed, but originals: true || false should be straightfoward and less to type

originals? What originals? originals: true is very very very misleading. The ideal name for the option is the name when you do not read the documentation and you understand what it does. It's strange to me to watch your desire to break this practice.

Why?

Because it is faster for me here (I do not see any problems in this).

Yes I will review asap. Which other developer, there sadly is none to review and sign-off instead (which should definitely be required for things like this)?

Working with other repositories, I came across the practice that if no one looks 72 hours, you can merge this (if you have permissions on this).

[michael-ciniawsky]

I don't think I'm violating anyting, since there never was an general agreement here...

uglifyOptions was an equally bad choice made, what else besides options should be there?
Everything on options is automatically an option, that's implied, otherwise following that logic consequently would mean to e.g use

const options = {
 cacheOption: ...
 filenameOption: ...
 xOptions: ...
}

instead?

originals? What originals? originals: true is very very very misleading. The ideal name for the option is the name when you do not read the documentation and you understand what it does. It's strange to me to watch your desire to break this practice.

To what other then the original assets/files should originals refer to here? I could ask the same questions about deleteOriginalAssets if I'm playing dumb originalsAssets? What are originalAssets? And we would I want to delete them?

The anwser is read the docs and (I doubt folks don't have to/or do so currently with options.deleteOriginalAsset aswell) and once you know what's going on a shorter name is likely easier to memorize and type in the future

Because it is faster for me here

That's likely not true

(I do not see any problems in this)

The problem is potentially inconsistency, we can't use just reword the commits instead to avoid that in the first place?

Working with other repositories, I came across the practice that if no one looks 72 hours, you can merge this (if you have permissions on this).

I disagree with this approach, for (critcal) bugfixes (time pressure) ok and smaller chores maybe, but definitely not for BREAKING CHANGES and features as there is no time pressure for those and since it influences the API at least ~1-2 approvals and reviews should be mandatory to land them

[alexander-akait]

@michael-ciniawsky

uglifyOptions was an equally bad choice made, what else besides options should be there?
Everything on options is automatically an option, that's implied, otherwise following that logic consequently would mean to e.g use

I disagree, third party options should be grouped in one object with prefix of third party. This is used by all the tools I've seen (not only on nodejs).

The anwser is read the docs and (I doubt folks don't have to/or do so currently with options.deleteOriginalAsset aswell) and once you know what's going on a shorter name is likely easier to memorize and type in the future

Disagree, options should be intuitive. When this is not so, people start to create new issue about misleading (Example importLoaders in css-loader). If something add/delete/change it should be prefixed with action.

That's likely not true

The problem is potentially inconsistency, we can't use just reword the commits instead to avoid that in the first place?

We did not use Conventional commits before this PR here, It does not make sense.

I disagree with this approach, for (critcal) bugfixes (time pressure) ok and smaller chores maybe, but definitely not for BREAKING CHANGES and features as there is no time pressure for those and since it influences the API at least ~1-2 approvals and reviews should be mandatory to land them

You are right here, but the main problem that I have already lost on empty discussions here is a long time (node version - we already discussion and we waste a lot of time on this, changing the names of options for a couple of characters, does not make sense at all, since this is a direct loss of time) and I do not want to continue this.

Your love in the constant renaming of options negatively affects the following things:

• Lost of time me and you.
• This often does not make sense, since the current name did not cause any problems (no issues about misleading).
• Constant discussions do not make the code better, but only slow down the release of more stable versions.
• People have to change the names of options after the update, although they still do the same.
• It's exhausting.
• We can endlessly disagree with each other for a long time.
• There are no serious arguments in favor of your proposed solutions (if you prefer something it is not mean it is right, past names did not cause any discomfort).
• We have a lot of work ahead of us. We lose the effectiveness of spending for this time.
• This is not a high priority.

I ask you to think about this. Often you are right in some things, but we have more priority tasks, and we do not need to waste time on reducing the names of options, discussion about webpack-defaults here (better do this in webpack-defaults repo) and similar discussions.

[michael-ciniawsky]

Okay better to leave the API discussion for now as we will never be able to reach consensus here as long as we have 2 persons and 2 different opinions colliding (this sadly blocks the PR then) We need to desparately focus on finding other people, to be able to reach consenus by a majority vote for this kind of disputes and to avoid additional/unnecessary conflict, since naturally every party holds to their point of view and neither will be 💯 right or wrong aswell

We did not use Conventional commits before this PR here, It does not make sense.

That's not true, of course do we use conventional commits here, please look at the CHANGELOG
I can rebase and reword the commits aswell if you're absolutely not willing to do it yourself...

You are right here, but the main problem that I have already lost on empty discussions here is a long time (node version - we already discussion and we waste a lot of time on this, changing the names of options for a couple of characters, does not make sense at all, since this is a direct loss of time) and I do not want to continue this.

Your love in the constant renaming of options negatively affects the following things:

It's not a good pratice trying to neglect other peoples opinions as irrational emotions, just because you disagree with them

Lost of time me and you.
This often does not make sense, since the current name did not cause any problems (no issues about misleading).
Constant discussions do not make the code better, but only slow down the release of more stable versions.
People have to change the names of options after the update, although they still do the same.
It's exhausting.
We can endlessly disagree with each other for a long time.
There are no serious arguments in favor of your proposed solutions (if you prefer something it is not mean it is right, past names did not cause any discomfort).
We have a lot of work ahead of us. We lose the effectiveness of spending for this time.
This is not a high priority.

I ask you to think about this. Often you are right in some things, but we have more priority tasks, and we do not need to waste time on reducing the names of options, discussion about webpack-defaults here (better do this in webpack-defaults repo) and similar discussions.

Some of these statements really begin to piss me off as most of them are just rude, overly personal biased and especially the constant 'my time is wasted here' argumention is simply inappropiated to state and I simply don't care for that matter, since I'm also investing my time here. How do you excatly think you are in that regard? Is your time more valuable then those of others and if you think so why? I remember we already had the excat same argument in postcss-loader a while ago and I already mentioned that this doesn't affect you alone. It is your responsibility to manage your own time and simply saying e.g "Sry, but I don't think any progress is currently being made here, so I'm doing xyz instead for now/meanwhile" would be perfectly fine and understandable, instead of whining and complaining about it all the time...

It's exhausting.
We can endlessly disagree with each other for a long time.

Yes agreed, see the first section of this comment why this likely is hard to resolve/unresolvable atm, when there is definite disagreement taking place. That still doesn't mean to claim other peoples opinions are mainly emotionally based (love, desire...) without reason or are simply a 'waste of time' to discuss, nor does it mean having to put pressure on others to magically resolve the dispute within the next e.g 72 hours or it simply doesn't matter. This PR seems to contain 0 (critical) bugfixes and therefore I'm unable to grasp the objective reasons why there should be any time pressure to release it asap. The only 'pressure' is to somehow reach consenus on which of the proposed changes both of us suggested are favored and should therefore land with this PR

In general the last PR reviews where getting pretty depressing and I would suggest to simply block PR's like this then and leave them for the time being. I'm not willing to continue reviewing PRs in that kind of manner I will just refrain from doing so otherwise and/or until we have found a solution for constructive dispute resolving. Secondly we should definitely stop spamming PR threads with our quarreling. Following and forming an opinion about this PR is likely already pretty hard for another person, we may even mark some of the comments here as 'off-topic' or better delete them to reduce the noise?

[alexander-akait]

@michael-ciniawsky I can look rude because I'm really tired of these discussions 😞 I apologize if something offended you, I did not want this.

This PR seems to contain 0 (critical) bugfixes and therefore I'm unable to grasp the objective reasons why there should be any time pressure to release it asap. The only 'pressure' is to somehow reach consenus on which of the proposed changes both of us suggested are favored and should therefore land with this PR

Because this is an abandoned PRs, it is bad. We already have these problem in past. And now I will not allow this to happen, we have to finish one to move on to the other, because this is the efficiency. Also, the internal wepback api can change and this will increase size of the next major release and bring more problems and regressions. We can produce any quantity major releases and it is normal.

My suggestion:

• Avoid discussion around node version, issue templates and other same problems in same PR (let's open new issue in webpack-defaults and after we come to a decision we will update it here.)
• Avoid unnecessary changes (no one faced the fact that did not understand how the option works, there is no reason to rename)
• Concentrate on more important issues (example webpack-dev-server and webpack-serve)
• Trust each other's solutions (We can always fix errors, change behavior and etc, without feedback from other developers there is no point in arguing with each other)

I do not see any reason not to release new major version. You are mistaken in saying that there are no important problems here. Example last cacache version contains fixes for windows (in some cases) and increase perf (it is really important). Package find-cache-dir also increase perf (drop node@4 code increase perf). Also we remove here unnecessary option asset. Less options - better maintenance.

[alexander-akait]

From my point:
I have a lot of issues in difference repos, I also have a regular job. I have boards in which I build my tasks for a day, a week, a month. Only after solving the problem I turn to the next. This allows you to lose nothing and not forget about anything. This allows me to effectively spend my time. At the moment, due to above discussions, I had to transfer part of the tasks. Because of this, much on my list has moved down and I will not have time to fulfill other tasks on time. This is very bad. In relation to me, this looks like a disrespect for my opinion and my time.

[alexander-akait]

Just explain the real reason for permanent changes name of options.

Cons:

• New name.

Props:

• I do not see any open issues.
• This really makes it harder for migrations between updates, because you have to constantly go and change the old options to new ones. But their behavior remains the same.
• This gives rise to disputes in which no one will ever be right.
• This increases the size of PR (it makes harder review)
• It is increase count of abandoned PRs

[montogeek]

It should be called compressionOptions, it is the name pattern webpack plugin/loaders uses.
Examples: Uglify, Terser, Babel Minify.

Also, compress name is counter intuitive, if I read that option I would say it is a boolean value, when you read ...options, you automatically know that it is an object to provide more configuration.

We also have to remember that this documentation is not only used by native English speakers or by Experienced JS/X language developers, we have to make sure it is accessible, intuitive at first sight and easier to read.

By naming it compresionOptions it would follow Uglify and Terser plugins API, which in the long term help our users experience and familiarity with webpack plugin ecosystem.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@ccb8aa6). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master   #109   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      2           
  Lines             ?     74           
  Branches          ?     25           
=======================================
  Hits              ?     74           
  Misses            ?      0           
  Partials          ?      0

Impacted Files	Coverage Δ
src/index.js	100% <100%> (ø)

---

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 ccb8aa6...6c58edc. Read the comment docs.

[michael-ciniawsky]

@montogeek kk thx for chiming in

It should be called compressionOptions, it is the name pattern webpack plugin/loaders uses.
Examples: Uglify, Terser, Babel Minify.

By naming it compresionOptions it would follow Uglify and Terser plugins API, which in the long term help our users experience and familiarity with webpack plugin ecosystem.

I argued against this in uglifyjs-webpack-plugin aswell and still think it was a 'mistake' made there. terser-webpack-plugin builds upon the same codebase as uglifyjs-webpack-plugin and babel-minify-webpack-plugin doesn't seem to follow that convention atm tbh

Also, compress name is counter intuitive, if I read that option I would say it is a boolean value, when you read ...options, you automatically know that it is an object to provide more configuration.

What is untiutive and especially counter intiutive in particular? I don't think it's possible to infer the value type for an particular options key solely by it's name and don't understand why that should have any benefits since it's likely one has to consult the README to know what props this particular {Object} has anyways. Having to read the documentation shouldn't be seen as an anti-pattern...

We also have to remember that this documentation is not only used by native English speakers or by Experienced JS/X language developers, we have to make sure it is accessible, intuitive at first sight and easier to read.

kk, please elaborate what is inaccessable, unituitive and why e.g compress(ion) is harder to read as compressOptions in particular please. I'm not a native speaker myself and the former is definitely easier to write and memorize imho. From a reading point of view

// for e.g 
options.compress.level
options.compression.level
options.compressOptions.level

the Options 'suffix' just feels kind of redundant to me personally aswell. It doesn't add anything from my point of view, since I'm aware we are dealing with options/configuration here and it's in both cases impossible to know the particular shape without reading the docs?

---

Anyways as it currently stands we have consensus to continue with compressOptions

[alexander-akait]

@michael-ciniawsky I'm tired of your endless debate and discussions. Now you do not agree with two people. Ignore my arguments so that you prefer other (name of option, formatting readme and etc) without issues/feedback/problems. Constant disputes only generate emptiness, creating a pseudocode does not solve any problems. At my request to review you answer I will do it later which reduces the effectiveness of development. I will continue my work and do major release today, if you do not like something you can create a issue about it. Sorry, but I can not continue to work using your approach to development. For all the time I worked alone, I wrote a lot of code and fixed a lot of errors, now I hear only groundless criticism of every line my code (by the way no one had complained on my code before).

In the past, you created a lot of PR that were then abandoned, to my questions why they were abandoned, you blamed that we are not organized enough. The problems are not in the organization but in the fact that it is useless to argue with you, you will always find something to complain about, and more often than not it has no practical reasons (as i said above - no issues/problems/PRs/etc).

https://opensource.guide/building-community/#dont-tolerate-bad-actors.