Skip to content

Commit 6a8f099

Browse files
committed
Fix the plugin return object for keep-enabled gzipping
In v0.2.0 we changed the return object for the plugin so that when the `keep` option is enabled we set a `distFiles` value in addition to gzippedFiles. This ensured our gzipped files got picked up by later plugins (e.g the manifest, S3 upload etc). Javascript passes objects by reference. You can see in the result object we were using the same `gzippedFiles` array object returned from the _gzipFiles Promise map. This turned out to be a bad idea. Setting distFiles equal to the array of gzipped files works because the return object from each plugin in the pipeline is merged with the pipeline's context, effectively concatenating the pipeline's `distFiles` with our `gzippedFiles` array. So far, so logical. The merge of the result with the existing context object is performed using lodash's `merge` function. This function maintains a stack of object values already merged to avoid cyclical object merging issues. As each key in the object to be merged is processed lodash looks up the key's value in the stack. If it has been seen before then the merge is cut short and the merge result's value for the given key is set to the value of the merge from the earlier key.. What this means for the gzip plugin is that after the result object is merged into the existing context by Ember CLI Deploy's pipeline model, `context.distFiles` === `context.gzippedFiles` - they are the exact same object. This is obviously incorrect and has repercussions later on e.g. the S3 plugin ends up setting `Content-Encoding: gzip` on every file it uploads. This would have been a whole lot easier to spot if I reordered the keys in the result object during my initial PR i.e. if I put `distFiles` below `gzippedFiles`. In that case the merge would end up destroying the context's `distFiles` and both would just contain the list of gzipped files. I've verified the problem still happens with Lodash 4.5.1 so I'm going to go talk to them. We're going to sidestep the problem here by not using the same object for both result keys, using a simple concat to build a new object.
1 parent e7969c4 commit 6a8f099

File tree

2 files changed

+12
-2
lines changed

2 files changed

+12
-2
lines changed

index.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,9 @@ module.exports = {
5959
if (keep) {
6060
self.log('keep is enabled, added gzipped files to `context.distFiles`', { verbose: true });
6161
return {
62-
distFiles: gzippedFiles,
62+
distFiles: [].concat(gzippedFiles), // needs to be a copy
6363
gzippedFiles: gzippedFiles
64-
}
64+
};
6565
}
6666
return { gzippedFiles: gzippedFiles };
6767
})

tests/unit/index-nodetest.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,16 @@ describe('gzip plugin', function() {
184184
done(reason);
185185
});
186186
});
187+
188+
it('does not use the same object for gzippedFiles and distFiles', function(done) {
189+
return assert.isFulfilled(plugin.willUpload(context))
190+
.then(function(result) {
191+
assert.notStrictEqual(result.distFiles, result.gzippedFiles);
192+
done();
193+
}).catch(function(reason){
194+
done(reason);
195+
});
196+
});
187197
});
188198
});
189199
});

0 commit comments

Comments
 (0)