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

Emit deep imports for helpers when using Common JS #7144

Closed
andersekdahl opened this issue Mar 25, 2023 · 7 comments · Fixed by #7147
Closed

Emit deep imports for helpers when using Common JS #7144

andersekdahl opened this issue Mar 25, 2023 · 7 comments · Fixed by #7147
Milestone

Comments

@andersekdahl
Copy link

Describe the feature

Currently when using external helpers SWC will output something like this for ESM:

import _create_class from "@swc/helpers/src/_create_class.mjs";

And something like this for CJS:

var _create_class = require("@swc/helpers").create_class;

I discovered this when analyzing this issue: webpack/webpack#16861

While I do believe that issue to be a webpack issue and not a SWC issue it would most likely not have happened if SWC emitted deep require statements instead of requireing from the entry point which exports all helpers. Eg, if the require statement would have looked like this:

var _create_class = require("@swc/helpers/lib/_create_class.js").default;

This would also have the added benefit of getting smaller bundles when using CJS since you'd only import the helpers actually used. To my knowledge webpack won't be able to correctly tree shake unused requires from lib/index.js.

Babel plugin or link to the feature description

No response

Additional context

No response

@magic-akari
Copy link
Member

The root cause: The exported function name is createClass instead of create_class.

Solution:
To reduce the complexity of conversion, I believe we should always use snake_case in both file name and exported function name.
In addition, named exports are less problematic than default exports, so I prefer to change all to named exports.

What are your thoughts? @kdy1

@kdy1
Copy link
Member

kdy1 commented Mar 26, 2023

I agree, camelCase isn't required at all.
Can we do it without breaking apps? We currently have no validation for the version of @swc/helpers.

@andersekdahl
Copy link
Author

andersekdahl commented Mar 26, 2023

Ah, didn't notice that there was a mismatch between the used name and the exported name. However, even if I manually patch @swc/helpers/lib/index.js to export the names that the require statement expect webpack/terser still strips it out completely. But I guess that's a webpack issue and not a SWC issue.

@magic-akari
Copy link
Member

I agree, camelCase isn't required at all.
Can we do it without breaking apps? We currently have no validation for the version of @swc/helpers.

We can declare peerDependencies in the package.json of @swc/helpers. Would this approach suffice in validating the versions?

"peerDependencies": {
    "@swc/core": ">1.3.42"
}

@kdy1
Copy link
Member

kdy1 commented Mar 26, 2023

Good idea. I never thought that I can use it to do such thing

@magic-akari
Copy link
Member

I think I should keep using default exports.
so there will be no breaking change.

@kdy1 kdy1 modified the milestone: Planned Mar 27, 2023
@kdy1 kdy1 added this to the Planned milestone Mar 28, 2023
kdy1 pushed a commit that referenced this issue Mar 31, 2023
@kdy1 kdy1 modified the milestones: Planned, v1.3.45 Apr 4, 2023
@swc-bot
Copy link
Collaborator

swc-bot commented May 4, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

4 participants