django-webpack / webpack-bundle-tracker

Spits out some stats about webpack compilation process to a file
MIT License
268 stars 107 forks source link

Add support for webpack 4 code splitting #37

Closed scdekov closed 5 years ago

scdekov commented 6 years ago

This change adds entries key in the result file containing list of all related chunks to that entrypoint.

david-house-harvard commented 6 years ago

@owais Thank you for all of your work in this project. We just started using it for our django project with webpack 4 and implementing code splitting would be a huge help.

owais commented 6 years ago

Thanks David. Is this backward compatible with previous version of webpack?

david-house-harvard commented 6 years ago

I'm not sure, but I imagine it would be because it's only additive. @scdekov 's work doesn't appear to affect any existing functionality.

I also haven't looked into the integration with your python project, https://github.com/owais/django-webpack-loader. For my project to be squared away, there might need to be changes there as well to take advantage of the new entries endpoint that's proposed by @scdekov

owais commented 6 years ago

I mixed up your comment with the main PR comment @david-house-harvard. Sorry about that.

@scdekov Can you please clarify if the changes are backward compatible with previous versions of webpack?

scdekov commented 6 years ago

@owais Yes the change should be backward compatible. It doesn't change the chunks key of the result object.

scdekov commented 6 years ago

@david-house-harvard I added support in the django-webpack-loader too. You can check it here https://github.com/scdekov/django-webpack-loader . It may need some refactoring, but it works pretty well for us.

david-house-harvard commented 6 years ago

That's great news, @scdekov , I suppose I should have assumed that you would have closed the loop already. If you or @owais require any help getting this over the line, I'm happy to carve out some time in our next sprint to help out.

scdekov commented 6 years ago

@david-house-harvard Yes, we've closed the loop, but I'll be glad if these changes are merged in the main repos.

david-house-harvard commented 6 years ago

@owais Is there anything that we can do to help move this along?

scdekov commented 6 years ago

Unfortunately I found problem with the way chunk paths are build when using it with django-loader. The problem is that in my version of the webpack-bundle-tracker, the paths are build relatively to the staticfiles directory, but IMO there need to be only the filenames and not paths. That way django loader can build the absolute paths by using the names and the bundles_dir setting. I have a fix of that and will to try to push it the following days.

harrywhite4 commented 6 years ago

Any updates on this? would be a really nice feature to have. If not i'm happy to look into it

alihazemfarouk commented 6 years ago

Having not seen this PR, I created another one that adds the key entryPoints to the result file.. You can check it out here. Then, I forked @scdekov version of django-webpack-loader and modified it to work with that PR (still missing tests). You can check it out here.

Feedback is appreciated

owais commented 6 years ago

If someone could update both the projects with proper tests, documentation and make sure at least django-webpack-loader would continue to work for version of webpack, I'd be happy to cut new releases and bump the major version if required.

scdekov commented 6 years ago

@alihazemfarouk I have some comments about the webpack-bundle-tracker PR. The change in django-webpack-loader looks good to me.

Freakazo commented 5 years ago

@scdekov thanks for you work!

[Edit]: Nevermind, I just realised we'll be working off from your and Alihazemfarouks changes, where looking at the code it seems like it should filter.

For django-webpack-loader: {% render_bundle 'name' 'css' %} Would only add the css entries, however: {% render_entrypoint 'name' 'css' %} Includes both the js and the css files.

scdekov commented 5 years ago

@Freakazo I didn't quite get what you mean. I think the code snippet that you posted is correct and the the new version of bundle-tracker/django-loader should work that way.

Freakazo commented 5 years ago

@scdekov yep sorry, I edited my comment. I wasn't looking at the updated django-loader.

alihazemfarouk commented 5 years ago

@scdekov Thanks a lot for your feedback. I made some changes to address the issues you highlighted. Could you please check again?

scdekov commented 5 years ago

@Freakazo I got what you mean. IMO render_entrypoint tag should accept only one parameter- the name of the entrypoint and include all of the files related to that entrypoint, no matter of the extensions. Are you on the same opinion?

alihazemfarouk commented 5 years ago

@scdekov I don't think that's what he meant. But anyway, I don't share the same opinion. What's wrong with, for example, wanting to load all your css files for a particular endpoint in the head tag, and load your js in the body tag? I think this scenario should be supported. What do you think?

scdekov commented 5 years ago

@alihazemfarouk I think you are right.

alihazemfarouk commented 5 years ago

If someone could update both the projects with proper tests, documentation and make sure at least django-webpack-loader would continue to work for version of webpack, I'd be happy to cut new releases and bump the major version if required.

I updated django-webpack-loader and webpack-bundle-track with documentation and made sure they both would still work with older versions of webpack. That leaves the tests. I won't be available to work on them for a few days so if anyone wants to do it, please, be me guest. webpack-bundle-tracker django-webpack-loader

alihazemfarouk commented 5 years ago

@owais I think we should be good to go. Please review and tell me if there are changes needed. webpack-bundle-tracker django-webpack-loader

bhrutledge commented 5 years ago

@scdekov This PR seems to be superseded by #41. Can this one be closed?

scdekov commented 5 years ago

@bhrutledge Yes. I'm closing it.