Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes #22 -- Adds trackAssets option to plugin #24

Closed
wants to merge 7 commits into from

Conversation

audiolion
Copy link

Asset tracking is off by default, by passing the trackAssets boolean to the
plugin options, all assets output by webpack will be added as a chunk with the
default chunk name assets to the webpack-stats.json file.

This enables rendering of files that are separately processed like gzipped js
and css from compress-webpack-plugin, fixing #22 .

Asset tracking is off by default, by passing the trackAssets boolean to the
plugin options, all assets output by webpack will be added as a chunk with the
default chunk name 'assets' to the webpack-stats.json file.

This enables rendering of files that are separately processed like gzipped js
and css from 'compress-webpack-plugin'.
@@ -42,7 +42,7 @@ module.exports = {
{
"status":"done",
"chunks":{
"app":[{
"app":[{
Copy link
Author

Choose a reason for hiding this comment

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

indentation was off by one space

output: {
path: require("path").resolve('./assets/bundles/'),
filename: "[name]-[hash].js",
publicPath: 'http://localhost:3000/assets/bundles/',
},

Copy link
Author

Choose a reason for hiding this comment

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

removed extra spaces in empty lines

@@ -67,7 +67,7 @@ And errors will look like,
{
"status": "error",
"file": "/path/to/file/that/caused/the/error",
"error": "ErrorName",
"error": "ErrorName",
Copy link
Author

Choose a reason for hiding this comment

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

removed extra spaces at the end of lines

@@ -7,12 +7,15 @@ var extend = require('deep-extend');
var assets = {};
var DEFAULT_OUTPUT_FILENAME = 'webpack-stats.json';
var DEFAULT_LOG_TIME = false;
var DEFAULT_ASSET_CHUNK_NAME = 'assets';
Copy link
Author

Choose a reason for hiding this comment

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

default name for trackAssets chunk, configurable in options



function Plugin(options) {
this.contents = {};
this.options = options || {};
this.options.filename = this.options.filename || DEFAULT_OUTPUT_FILENAME;
this.options.trackAssets = this.options.trackAssets || false;
Copy link
Author

Choose a reason for hiding this comment

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

trackAssets is off by default so there will be no change from default behavior of the app for those upgrading

@@ -57,7 +60,7 @@ Plugin.prototype.apply = function(compiler) {
var files = chunk.files.map(function(file){
var F = {name: file};
if (compiler.options.output.publicPath) {
F.publicPath= compiler.options.output.publicPath + file;
F.publicPath = compiler.options.output.publicPath + file;
Copy link
Author

Choose a reason for hiding this comment

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

missing space around operator

index.js Outdated

if (this.options.trackAssets) {
var assets = [];
Object.keys(stats.compilation.assets).map(function(asset){
Copy link
Author

Choose a reason for hiding this comment

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

similar to above function, goes over the assets json object instead

@audiolion
Copy link
Author

Had some typos in the readme, fixed them

* 'master' of github.com:audiolion/webpack-bundle-tracker:
  Update README.md
  Update README.md
  Update README.md
@GonzaloRizzo
Copy link

@owais What's stopping this from being merged?

@owais
Copy link
Collaborator

owais commented Sep 21, 2017

I don't remember exactly. IIRC, I wanted to explore handling this using a plugin. Anyway, @audiolion could you share the different versions of the generated manifest with and without the flag. The manifest is not supposed to be human readable so if including these extra files in the manifest does not cause any hindrances to the main use case of django-webpack-loader, we could set the flag to always include the files or even better, remove the flag completely and always include files.

@owais
Copy link
Collaborator

owais commented Sep 21, 2017

Here is another PR that does something similar I think. #17

I think what I wanted to do was to ship a bundle tracker loader so people could wrap their usual loaders with bundle tracker loader and that would include those files in the output of bundle tracker. That would give users much more flexibility and tight control over what to include and what not. If anyone has time to explore this idea and send a PR or just report back with the experience, it would be be very welcome.

@audiolion
Copy link
Author

Have been out of this for awhile now, I solved the issue in a less than desirable way by using an if statement in django templates and saying if I was in production to append .gz to the extension e.g. css.gz js.gz. I am not too clear on what your idea is with a bundle tracker loader, for me personally this solves the gzip problem for now, and doesn't cause additional technical debt if a later refactor is done.

I can post the diff of the bundles if you are still considering this PR

@owais
Copy link
Collaborator

owais commented Sep 21, 2017

Your solution sounds very good to me. If a simple if condition can solve a problem, it means the solution does not need to be pushed down the stack unless the solution becomes really unmanageable.

If anyone else wants to take a stab at this, checkout my comment about loaders above.

@owais owais closed this Sep 21, 2017
@swiharta
Copy link

swiharta commented Oct 12, 2017

Could you please explain how this if condition in a Django template allows you to append the .gz extension? I have coded a new gz variable into the render_bundle templatetag which allows me to check for it and add .gz in get_as_tags() that way, so my tag looks like {% render_bundle 'MyApp' gz='1' %}. I'd like to be able to delete that hack and do it somehow just within Django templates. Thank you for any help!

@audiolion
Copy link
Author

I actually use my fork with this PR incorporated and do

{% if debug %}
  {% render_bundle 'main' 'css.gz' %}
{% else %}
  {% render_bundle 'main' 'css' %}
{% endif %}

and have my output from webpack mapped to the main chunkName

@swiharta
Copy link

Got it, thanks! I should probably not be using more than one .js file for my site but currently do have a couple different named "entry" directories in my webpack.config.js, so I was hoping to keep the two different chunkNames as they are now and just add the extension as a third argument. I will probably just consolidate to one js file and do what you are doing, but an improvement to your fork might be to preserve the original chunkNames instead of creating a new chunkName (main in your case above, defaults to assets in your fork).

@audiolion
Copy link
Author

I think it does preserve the chunk names, like for me I have

{% render_bundle 'manifest' 'js' %}
{% render_bundle 'vendor' 'js' %}
{% render_bundle 'main' 'js' %}

all my app code is in the main chunk, vendor split out by CommonChunksPlugin is in vendor, and the same for manifest.

@swiharta
Copy link

swiharta commented Oct 12, 2017

I don't know how that can be working, for me all of my entry points / all of the bundles generated by your fork go under the "assets" name, and all get loaded with {% render_bundle "assets" %}. The relevant code is here:

      if (self.options.trackAssets === true) {
        var assets = [];
        Object.keys(stats.compilation.assets).map(function(asset){
          var F = {name: asset};
          if (compiler.options.output.publicPath) {
            F.publicPath= compiler.options.output.publicPath + asset;
          }
          if (compiler.options.output.path) {
            F.path = path.join(compiler.options.output.path, asset);
          }
          assets.push(F);
        });
        chunks[self.options.assetsChunkName] = assets;
      }

...where self.options.assetsChunkName seems to be a static string and defaults to "assets", unlike the preceding code (not your fork's code):

      var chunks = {};
      stats.compilation.chunks.map(function(chunk){
        var files = chunk.files.map(function(file){
          var F = {name: file};
          if (compiler.options.output.publicPath) {
            F.publicPath = compiler.options.output.publicPath + file;
          }
          if (compiler.options.output.path) {
            F.path = path.join(compiler.options.output.path, file);
          }
          return F;
        });
        chunks[chunk.name] = files;
      });

...where the chunk.name is a dynamic value apparently based on the different entry points. Is this incorrect?

Do I need to learn about CommonsChunkPlugin? The first part of my webpack.config.js file looks like this:

    entry: {
        Phone: './assets/js/phone/phone',
        Cme: './assets/js/cme/cme'
    },

    output: {
        path: path.resolve('./assets/bundles/'),
        filename: "[name]-[hash].js",
    },

@swiharta
Copy link

swiharta commented Oct 13, 2017

Here's my "dumb" fix for my current situation:

      var chunks = {};
      stats.compilation.chunks.map(function(chunk){
          var files = []
          chunk.files.map(function(file){
              [file, file+'.gz'].map(function(fileName){
                  var F = {name: fileName};
                  if (compiler.options.output.publicPath) {
                      F.publicPath = compiler.options.output.publicPath + fileName;
                  }
                  if (compiler.options.output.path) {
                      F.path = path.join(compiler.options.output.path, fileName);
                  }
                  files.push(F);
              });
          });
        chunks[chunk.name] = files;
      });

This just adds duplicated chunks but with .gz appended. It dumbly assumes these files exist when they may or may not, but it lets me do {% render_bundle 'Phone' 'js.gz' %}.

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

Successfully merging this pull request may close these issues.

4 participants