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.

Sign up for GitHub

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

Jump to bottom

Support reading large stats.json files #423

Merged
merged 2 commits into from Feb 13, 2023
Merged

Support reading large stats.json files #423

merged 2 commits into from Feb 13, 2023

Conversation

henry-alakazhang
Copy link
Contributor

@henry-alakazhang henry-alakazhang commented Mar 19, 2021 •
edited

Node has a string size limit of 500MB and the current JSON.parse(fs.readFileSync(...)) logic to read stats.json files fails for files >500MB.

This PR changes the approach for stats parsing to use a stream, similar to this PR for webpack-cli in creating the file: webpack/webpack-cli#2190

I've tested it against a local repo with a 700MB stats.json and it works when it didn't before. I would add a test, but I don't know about adding that large of a file 😂 Could look into generating one if you would like.

@tiberiuzuld tiberiuzuld mentioned this pull request Jun 29, 2022
Closed
@cblnco
Copy link

cblnco commented Nov 7, 2022 •
edited

Is there a possibility this feature gets merged @th0r, @valscion?
I know that the workaround would be to use the plugin instead of the CLI. However, I'm on a scenario where my project doesn't manages all the Webpack settings directly and I can't use this plugin.
It would be really really nice to use the CLI too in this case.

@valscion
Copy link
Member

valscion commented Nov 8, 2022

Yeah this looks like something that could work! I seem to have forgotten that this PR exists, sorry about that 😳

@henry-alakazhang would you be open to:

  • recreating this pull request on top of the current master branch
  • use the latest version of @discoveryjs/json-ext that exists
  • create a changelog entry as "Improvement" category
  • signing the CLA

We can make do without a test for this as long as the existing tests pass.

@gavinbarron
Copy link

@valscion this seems to be stalled, and it's something that we could make use of for tracking work to produce smaller app bundles when using kiota to build TypeScript SDKs for HTTP APIs.

If @henry-alakazhang is not able to progress this contribution I'd be happy to pick it up and move it forward.

@valscion
Copy link
Member

valscion commented Jan 18, 2023 •
edited

Yeah feel free to pick this change up move it forward @gavinbarron ☺️

@cblnco
Copy link

cblnco commented Feb 3, 2023

Now that I have bandwidth I could also help moving forward with this contribution @valscion, @henry-alakazhang, @gavinbarron.
I will keep an eye on this to know who ends up integrating these changes.

@henry-alakazhang
Copy link
Contributor Author

Hey whoops sorry, completely missed this. I'll update the PR.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 11, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: henry-alakazhang / name: Henry Zhang (e995d16)

@henry-alakazhang
Copy link
Contributor Author

henry-alakazhang commented Feb 11, 2023 •
edited

Updated with latest version of json-ext. I've refactored the code to use async/await because I wasn't sure how readStatsFromFile would actually throw errors.

Remains untested as I can't run yarn test for some reason - the npm i to install the test webpack versions doesn't want to run regardless of which Node version I'm using (erroring with Maximum call stack size exceeded).

edit: tests pass locally. also can confirm it still works with a large file

valscion
valscion approved these changes Feb 13, 2023
View reviewed changes
Copy link
Member

@valscion valscion left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great and the tests still pass in CI so I'll merge :)

@valscion valscion merged commit e231c77 into webpack-contrib:master Feb 13, 2023
@valscion
Copy link
Member

This is now in v4.8.0! 🚀

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Feb 14, 2023
Update dependency webpack-bundle-analyzer to v4.8.0 (#1783)
88e0faa 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [webpack-bundle-analyzer](https://github.com/webpack-contrib/webpack-bundle-analyzer) | devDependencies | minor | [`4.7.0` -> `4.8.0`](https://renovatebot.com/diffs/npm/webpack-bundle-analyzer/4.7.0/4.8.0) |

---

### Release Notes

<details>
<summary>webpack-contrib/webpack-bundle-analyzer</summary>

### [`v4.8.0`](https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/HEAD/CHANGELOG.md#&#8203;480)

[Compare Source](webpack-contrib/webpack-bundle-analyzer@v4.7.0...v4.8.0)

-   **Improvement**
    -   Support reading large (>500MB) stats.json files ([#&#8203;423](webpack-contrib/webpack-bundle-analyzer#423) by [@&#8203;henry-alakazhang](https://github.com/henry-alakazhang))
    -   Improve search UX by graying out non-matches ([#&#8203;554](webpack-contrib/webpack-bundle-analyzer#554) by [@&#8203;starpit](https://github.com/starpit))

-   **Internal**
    -   Add Node.js v16.x to CI and update GitHub actions ([#&#8203;539](webpack-contrib/webpack-bundle-analyzer#539) by [@&#8203;amareshsm](https://github.com/amareshsm))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [x] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMzMuMCIsInVwZGF0ZWRJblZlciI6IjM0LjEzMy4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1783
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@valscion valscion mentioned this pull request Feb 23, 2023
Closed
@Yexiaoxing Yexiaoxing mentioned this pull request Mar 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valscion valscion valscion approved these changes

Assignees
No one assigned
Labels
pr_action: Rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@henry-alakazhang @cblnco @valscion @gavinbarron