Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix integrity calculation, fixes #126 #127

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

karolyi
Copy link

@karolyi karolyi commented Jul 28, 2024

No description provided.

@karolyi
Copy link
Author

karolyi commented Jul 28, 2024

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
Copy link
Author

karolyi commented Jul 29, 2024

oh shit, webpack4 doesn't have assetEmitted.

@karolyi
Copy link
Author

karolyi commented Jul 30, 2024

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
Copy link
Author

karolyi commented Jul 30, 2024

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
Copy link
Member

fjsj commented Aug 2, 2024

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 #126 now. I'll have to check when integrity usage really broke, or if it does work in the examples.

@karolyi
Copy link
Author

karolyi commented Aug 2, 2024

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
Copy link
Author

karolyi commented Aug 2, 2024

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
Copy link
Member

fjsj commented Aug 2, 2024

  • Webpack 4 is still maintained. See open PRs in webpack repo, they mention webpack 4 and some features are still backported AFAIK, although it's been a year since the last release.
  • Standardized code formatting ensures minimal PRs, which ensures clean git history, which helps with git blame, git bisect, tracking regressions, etc. OK if you can't fix that, it's open-source, no warranty from any side.
  • No further improvements in this PR will make my job harder, as the PR has no tests, I will have to test things myself, reproduce the issue, ensure no breaking changes, and if the solution really solves the issue. I cannot give a timeframe on that.

@karolyi
Copy link
Author

karolyi commented Aug 2, 2024

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
Copy link
Author

karolyi commented Aug 2, 2024

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
Copy link
Author

karolyi commented Nov 9, 2024

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

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

Successfully merging this pull request may close these issues.

2 participants