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

Fixes #427, #556 use correct sourceType when parsing bundles #649

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

eamodio
Copy link
Contributor

@eamodio eamodio commented Jan 8, 2025

Refs #427, #556

We are now parsing bundles as ES modules based on stats JSON information

Copy link

linux-foundation-easycla bot commented Jan 8, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: eamodio / name: Eric Amodio (a0199c2)
  • ✅ login: valscion / name: Vesa Laakso (0d6e706)

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Sign the CLA please 🥰

@eamodio
Copy link
Contributor Author

eamodio commented Jan 8, 2025

Done 😀

@valscion
Copy link
Member

valscion commented Jan 9, 2025

Thanks! I wonder if there is any other solution than forcing the user to change an option? Is there some way we could infer whethet the source content is a module and not a script?

I'd rather avoid adding more options to support something which should work out-of-the-box.

@eamodio
Copy link
Contributor Author

eamodio commented Jan 10, 2025

Agreed, but not sure how to detect that. Ideas?

@alexander-akait
Copy link
Member

alexander-akait commented Feb 4, 2025

Each asset has javascriptModule: true in info when it is a ECMA module, so it can be infer from it (https://github.com/webpack-contrib/terser-webpack-plugin/blob/master/src/index.js#L490), I don't think using global parserOptions is good idea, because technically webpack can generates and ECMA modules and any other module system

@valscion
Copy link
Member

valscion commented Feb 4, 2025

Each assets has javascriptModule: true in info when it is a ECMA module, so it can be infer from it (https://github.com/webpack-contrib/terser-webpack-plugin/blob/master/src/index.js#L490), I don't think using global parserOptions is good idea, because technically webpack can generates and ECMA modules and any other module system

Oh that's nice! So the information coming from stats JSON should definitely be used here over a global option.

The first change could be to only change the sourceType and leave other stuff alone if it makes sense.

@alexander-akait
Copy link
Member

@eamodio Do you want to finish it, thank you?

@eamodio
Copy link
Contributor Author

eamodio commented Feb 10, 2025

Updated

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! I think this direction looks safe and seems to be backwards-compatible.

Can you modify the PR title, description and README.md to describe what has now changed?

For example, something like this:

"Parse bundles as ES modules based on stats JSON information"

@eamodio eamodio changed the title Fixes #427, #556 adds options to set sourceType Fixes #427, #556 use correct sourceType when parsing bundles Feb 10, 2025
@eamodio
Copy link
Contributor Author

eamodio commented Feb 10, 2025

@valscion Updated the title & description, but not sure what you wanted added to the README as there isn't any new options or anything. But feel free to edit this PR as you see fit.

@valscion
Copy link
Member

Ah sorry, I meant the changelog! My bad.

@eamodio
Copy link
Contributor Author

eamodio commented Feb 14, 2025

Updated -- hopefully this is ready now

@valscion
Copy link
Member

Can you avoid changing the formatting on all rows in the changelog? The diff there should be minimal.

Thank you for your patience with this PR. I'm mostly reviewing this on my phone so I can't easily modify the changes myself.

@eamodio
Copy link
Contributor Author

eamodio commented Feb 17, 2025

Ugh, sorry, damn auto-formatter. Fixed

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, thank you! ☺️

@valscion valscion merged commit b82e5fa into webpack-contrib:master Feb 18, 2025
2 of 6 checks passed
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.

4 participants