Deprecate cache parameter in favour of seed #57

nathanbirrell · 2017-07-11T05:03:39Z

Functionality for cache stays the same, the more generic name is better suited for when your app needs to set custom keys.

requested in #55

closes #25

codecov-io · 2017-07-11T05:10:08Z

Codecov Report

Merging #57 into master will increase coverage by 3.69%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   89.28%   92.98%   +3.69%     
==========================================
  Files           2        2              
  Lines          56       57       +1     
==========================================
+ Hits           50       53       +3     
+ Misses          6        4       -2

Impacted Files	Coverage Δ
lib/plugin.js	`92.85% <100%> (+3.76%)`	⬆️

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 a985131...357f3d7. Read the comment docs.

mastilver · 2017-07-11T09:23:19Z

lib/plugin.js

@@ -88,6 +90,9 @@ ManifestPlugin.prototype.apply = function(compiler) {
      }.bind(this), {});
    }

+    // Add custom values last, so they don't get the basePath prefixed to them
+    _.merge(manifest, initValue);


use manifest = _.assign({}, initValue, manifest)

mastilver · 2017-07-11T09:25:13Z

spec/plugin.spec.js

@@ -214,6 +214,58 @@ describe('ManifestPlugin', function() {
      });
    });



Can you add a test to prove that initValue is being overwritten

mastilver · 2017-07-11T09:28:22Z

@a-x- @zuzusik @gmac Does initValue make sense to you (I had no idea how to name it so I just looked at the doc for Array.reduce :D )

gmac

I'm 👎 on this as a feature. This capability is already possible using cache, which handles seeding a manifest with initial key value pairs. That would be the correct way to achieve this.

Admittedly, the readme docs only mention cache in the context of multi-compiler architecture (which is a less common use case). I'd propose changing this PR to be documentation-only with the cache article updated to denote: "A cache of key/value pairs to used to seed the manifest. This may include a set of custom key/value pairs to include in your manifest, or may be used to combine manifests across compilations in [multi-compiler mode]."

…ache

nathanbirrell · 2017-07-13T00:37:22Z

Hey @gmac, I agree.

I've updated the PR, unit tests and readme to reflect the move to seed (mentioned here).

It will still accept cache for now.

Cheers

mastilver · 2017-07-13T08:18:59Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "webpack-manifest-plugin",
-  "version": "1.1.2",
+  "version": "1.1.4",


Please leave the version as is :)

mastilver · 2017-07-13T08:21:08Z

@gmac are you ok with those changes?
Or shall we output a deprecation warning using util.deprecate?

zuzusik · 2017-07-14T16:18:27Z

spec/plugin.spec.js

+      });
+    });
+
+    it('seed attributes do not get prefixed with basePath', function(done) {


I think we should go here with does something style - does not prefix seed attributes with basePath

Nice one, done.

zuzusik · 2017-07-14T16:21:50Z

spec/plugin.spec.js

    it('combines manifests of multiple compilations', function(done) {
+      var seed = {};
+      webpackCompile([{


do we need this var at all?

shouldn't we place a new line after var?

zuzusik · 2017-07-17T17:13:30Z

README.md

@@ -52,4 +55,4 @@ new ManifestPlugin({
 * `publicPath`: A path prefix used only on output files, similar to Webpack's  [output.publicPath](https://github.com/webpack/docs/wiki/configuration#outputpublicpath). Ignored if `basePath` was also provided.
 * `stripSrc`: removes unwanted strings from source filenames (string or regexp)
 * `writeToFileEmit`: If set to `true` will emit to build folder and memory in combination with `webpack-dev-server`
-* `cache`: In [multi-compiler mode](https://github.com/webpack/webpack/tree/master/examples/multi-compiler) webpack will overwrite the manifest on each compilation. Passing a shared `{}` as the `cache` option into each compilation's ManifestPlugin will combine the manifest between compilations.
+* `seed`: A cache of key/value pairs to used to seed the manifest. This may include a set of [custom key/value](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json) pairs to include in your manifest, or may be used to combine manifests across compilations in [multi-compiler mode](https://github.com/webpack/webpack/tree/master/examples/multi-compiler).


Should we add here the information how exactly it should be used in multi-compiler-mode?

Maybe I am missing something, but for me it is not really clear that I need to specify it as an empty object to do that - I cleared this out from tests, but who is going to read tests when he needs info?

How about just adding: To combine manifests, pass a shared seed object to each compiler's plugin instance.

To combine manifests, pass a shared seed object to each compiler's plugin instance.

this can be somehow confusing for those not knowing how it all works under the hood - the first question will be what is compiler plugin instance?

I assume adding : just pass it as empty object: 'seed: {}' before final .

I find just pass it as empty object: 'seed: {}' more confusing than helpful. It doesn't make that point that a shared singleton object must be passed to multiple plugin instances to combine their manifests. I amend my earlier text for clarity:

To combine manifests, pass a shared seed object to each compiler's ManifestPlugin instance.

If you're doing multi-compiler architecture, that should make sense. If you're not, you don't need the information anyway.

gmac

Adding that extra bit of docs about a shared compiler instance would be helpful. Overall though, this looks good. 👍

gmac · 2017-07-17T17:58:05Z

README.md

@@ -52,4 +55,4 @@ new ManifestPlugin({
 * `publicPath`: A path prefix used only on output files, similar to Webpack's  [output.publicPath](https://github.com/webpack/docs/wiki/configuration#outputpublicpath). Ignored if `basePath` was also provided.
 * `stripSrc`: removes unwanted strings from source filenames (string or regexp)
 * `writeToFileEmit`: If set to `true` will emit to build folder and memory in combination with `webpack-dev-server`
-* `cache`: In [multi-compiler mode](https://github.com/webpack/webpack/tree/master/examples/multi-compiler) webpack will overwrite the manifest on each compilation. Passing a shared `{}` as the `cache` option into each compilation's ManifestPlugin will combine the manifest between compilations.
+* `seed`: A cache of key/value pairs to used to seed the manifest. This may include a set of [custom key/value](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json) pairs to include in your manifest, or may be used to combine manifests across compilations in [multi-compiler mode](https://github.com/webpack/webpack/tree/master/examples/multi-compiler).


How about just adding: To combine manifests, pass a shared seed object to each compiler's plugin instance.

zuzusik · 2017-07-17T20:06:10Z

Thinking about further improvements here: is there a way to detect that we are in multi-compiler mode inside of a plugin and then switch to seeding mode?

This would be really great and convenient - no need to specify seed as {} - it will just work out of the box.

gmac · 2017-07-17T20:29:13Z

Sure, it's possible to detect multi-compiler mode... you just look at the number of compilers configured. Off the top of my head, I think that's managed by an array called compilers on the root Webpack compiler object.

This sounds like a potentially useful feature, although beyond the scope of this PR. There's definitely some nuances there that would need to be solved for. The way I'd see it working would be to pass an option like universalManifest: true. When enabled, a plugin instance would configure around an internal singleton and assign any provided seed params onto that. Otherwise, it would configure around the provided seed object itself. The nuance would be in tracking the right object identity.

nathanbirrell · 2017-07-18T23:05:01Z

Thanks all. Updated the readme with @gmac's suggestion 👍🏼

gmac

LGTM 👍

mastilver · 2017-07-20T15:57:54Z

👍 I will release that tonight

Add initValue object

bc3b349

nathanbirrell mentioned this pull request Jul 11, 2017

Add reduce, initValue, map, filter options #55

Closed

4 tasks

nathanbirrell added 5 commits July 11, 2017 15:37

Move initValue object merge to last to avoid basePath prefixing

d1f83b3

Test initialValue object

854711b

Add test to check initialValue attributes don't get prefixed by basePath

078f124

Update readme

970c1d0

Add example initValue to readme

cb5879a

mastilver suggested changes Jul 11, 2017

View reviewed changes

gmac suggested changes Jul 11, 2017

View reviewed changes

nathanbirrell added 4 commits July 13, 2017 10:18

Revert initValue work in favour of cache, modify unit tests to test c…

d5c876a

…ache

bump

1bd93b9

Deprecate cache paramater in favour of seed

ad6e29d

Bump and update readme

f3027d3

nathanbirrell changed the title ~~Add initValue object~~ Deprecate cache parameter in favour of seed Jul 13, 2017

Update seed explanation in readme

919e955

nathanbirrell mentioned this pull request Jul 13, 2017

Define other manifest fields #25

Closed

mastilver suggested changes Jul 13, 2017

View reviewed changes

Revert version bumps

7efdcf3

mastilver approved these changes Jul 14, 2017

View reviewed changes

zuzusik reviewed Jul 14, 2017

View reviewed changes

Reword basePath prefix test

afd2640

zuzusik reviewed Jul 17, 2017

View reviewed changes

gmac approved these changes Jul 17, 2017

View reviewed changes

Add multi-complier explanation to readme

357f3d7

mastilver approved these changes Jul 18, 2017

View reviewed changes

gmac approved these changes Jul 20, 2017

View reviewed changes

gmac merged commit 56cfa00 into shellscape:master Jul 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate cache parameter in favour of seed #57

Deprecate cache parameter in favour of seed #57

nathanbirrell commented Jul 11, 2017 •

edited

Loading

codecov-io commented Jul 11, 2017 •

edited

Loading

mastilver Jul 11, 2017

mastilver Jul 11, 2017

mastilver commented Jul 11, 2017

gmac left a comment •

edited

Loading

nathanbirrell commented Jul 13, 2017 •

edited

Loading

mastilver Jul 13, 2017

nathanbirrell Jul 13, 2017

mastilver commented Jul 13, 2017

zuzusik Jul 14, 2017

nathanbirrell Jul 16, 2017

zuzusik Jul 14, 2017

zuzusik Jul 17, 2017 •

edited

Loading

gmac Jul 17, 2017

zuzusik Jul 17, 2017

gmac Jul 17, 2017

gmac left a comment

gmac Jul 17, 2017

zuzusik commented Jul 17, 2017

gmac commented Jul 17, 2017 •

edited

Loading

nathanbirrell commented Jul 18, 2017

gmac left a comment

mastilver commented Jul 20, 2017

		@@ -214,6 +214,58 @@ describe('ManifestPlugin', function() {
		});
		});

Deprecate cache parameter in favour of seed #57

Deprecate cache parameter in favour of seed #57

Conversation

nathanbirrell commented Jul 11, 2017 • edited Loading

codecov-io commented Jul 11, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastilver commented Jul 11, 2017

gmac left a comment • edited Loading

Choose a reason for hiding this comment

nathanbirrell commented Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastilver commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuzusik Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuzusik commented Jul 17, 2017

gmac commented Jul 17, 2017 • edited Loading

nathanbirrell commented Jul 18, 2017

gmac left a comment

Choose a reason for hiding this comment

mastilver commented Jul 20, 2017

nathanbirrell commented Jul 11, 2017 •

edited

Loading

codecov-io commented Jul 11, 2017 •

edited

Loading

gmac left a comment •

edited

Loading

nathanbirrell commented Jul 13, 2017 •

edited

Loading

zuzusik Jul 17, 2017 •

edited

Loading

gmac commented Jul 17, 2017 •

edited

Loading