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 .cjs files not showing up in bundle analyzer #512

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

Rush
Copy link
Contributor

@Rush Rush commented Jul 17, 2022

I compile my server-side code to .cjs files. I was surprised to find the bundle analyzer view to be completely empty.

After debugging it turned out that .cjs filename was not being handled.

I think in general all assets should be included in the analysis but .cjs is good for now!

Could you release this patch on npm as well?

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 17, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Rush / name: Damian Kaczmarek (da87ff0, e14a676, dcb5ade)
  • ✅ login: valscion / name: Vesa Laakso (710fa67)
  • ✅ login: alexander-akait / name: Alexander Akait (667bd16)

@Rush
Copy link
Contributor Author

Rush commented Jul 17, 2022

Btw. I signed CLA. You can re-run your CI.

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM

@Rush
Copy link
Contributor Author

Rush commented Jul 17, 2022

Thanks! :) I would appreciate if it could be released in the nearby future.

@valscion
Copy link
Member

Looks good, thanks! Are you able to create a small test to verify this won't regress in the future? Also a mention of this change in changelog would be nice 😊

@valscion
Copy link
Member

Hi @Rush are you still interested in getting this PR done? We'd need this PR to be rebased on top of latest master and a small test added to verify it won't regress in the future. A changelog entry would also be nice.

@alexander-akait
Copy link
Member

@valscion Hello, can we resolve this PR? Because a got a lot of questions why .cjs doesn't work

@valscion
Copy link
Member

valscion commented Apr 9, 2024

Yeah a PR which adds a minimal test case showing that .cjs files work, adds a changelog entry and does this same change will be accepted.

@alexander-akait
Copy link
Member

Okey, I will resend it 👍

@Rush
Copy link
Contributor Author

Rush commented Apr 9, 2024

Sorry I don't have any bandwidth to work on it. One solution to consider would be to merge this and create a follow up issue with a test case. I've been using my .cjs branch for almost two years now :-)

@valscion
Copy link
Member

Yeah now that this PR is up-to-date then adding some test coverage should hopefully be quite simple.

Would you @Rush have bandwidth to create a changelog entry for this change still?

@Rush
Copy link
Contributor Author

Rush commented Apr 10, 2024

@valscion let me know if this is what you had in mind

@alexander-akait
Copy link
Member

@valscion Added a test case

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.

Looks great, thanks!

@valscion valscion merged commit cbc26b0 into webpack-contrib:master Apr 11, 2024
6 checks passed
@valscion
Copy link
Member

This has now been released as part of v4.10.2

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

Successfully merging this pull request may close these issues.

5 participants