django-webpack / webpack-bundle-tracker

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

Fix integrity calculation, fixes #126 #127

Open karolyi opened 3 months ago

karolyi commented 3 months ago

So the basic idea is to calculate the hashes within the _handleAssetEmitted hook and not in _handleDone.

It seems that _handleDone still doesn't have the files written to the disk when running. Not to mention the dev-server case, when the files don't even get written to the disk.

_handleAssetEmitted is able t get the rendered content and thus is able to create an integrity hash of it.

karolyi commented 3 months ago

oh shit, webpack4 doesn't have assetEmitted.

karolyi commented 3 months ago

Okay, I don't think anyone uses this with webpack4 because I've found multiple code paths that would simply just bail out when using webpack 4.

Also, tests are a disaster. Spent a couple hours of trying to get them working but to no avail. Who thought expect.toBePositive() is a valid and passable expect statement?

Anyways, tests are FUBAR, I won't even try to get them in order anymore.

There is a fix I need to make (cleaning up the irrelevant chunk filenames) and then this PR is good to go.

karolyi commented 3 months ago

Okay, should be good to go now. I know it works with webpack 5 because I'm currently using it, as in I updated it for myself.

A timely merge and release is highly advised.

fjsj commented 3 months ago

Thanks for the PR. I approved the runs, please fix the code-style issues, otherwise it will break blame.

Who thought expect.toBePositive() is a valid and passable expect statement?

Well, just do a git blame to figure it out :) But seriously, we're happy to improve anything on tests, feel free to open other PRs that address this, or we can discuss in a new issue.

Could you please clarify what exactly is broken in integrity calculation? The way to clarify this is to write a regression test that breaks before your change.

EDIT: I see the issue https://github.com/django-webpack/webpack-bundle-tracker/issues/126 now. I'll have to check when integrity usage really broke, or if it does work in the examples.

karolyi commented 3 months ago

Thanks for the feedback.

You could just approve the runs without the code formatting enforcement. I'll try to be polite here, so please don't take personally what I have to say: I've came to the conclusion to refuse to contribute more than the absolute minimum to projects that enforce code formatting on my or someone else's code.

It is a personal opinion, so take it or leave it, just like this contribution.

karolyi commented 3 months ago

Btw there is no solid statement on if this module supports webpack 4 (which I think is EOL now). Would that have been the case, the changes would have been way easier. I've spent a couple days just to make this compatible with webpack4 just by going with the type definitions, but haven't tested it — other than running the failing tests which proved webpack4 still works.

fjsj commented 3 months ago
karolyi commented 3 months ago

No worries, I have my own fork used and working. It is really hard to prove a negative here since the module already works with the positive and not the other way around - as is the purpose of this PR.

I'll get notified if this PR ever merges, until then I'll just use mine.

karolyi commented 3 months ago

As for the code formatting part, yeah I may have been a bit overreaching in reformatting parts I didn't touch on, but it's already done. It was a mess to work with the module (not because of the module, but because of JS), so I had to do my best to get a birds-eye-view of it.

karolyi commented 2 days ago

Bumping for a timely review and a merge. FYI, I'm using this successfully in my projects.