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

feat: preserve class names when renaming declarations #508

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

Conversation

Demivan
Copy link
Contributor

@Demivan Demivan commented Mar 9, 2024

Description

Preserve class names when fixing naming collisions.

When variable name clashes:

const a = class {}

becomes:

const a$1 = class a {}

When class name clashes:

class A {}

becomes:

var A$1 = class A {} 

Test Plan

Enabled rollup@function@class-name-conflict test.

This is my first actual Rust PR, so there could be issues.


Copy link

netlify bot commented Mar 9, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit dbf0615
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/65ec8548d2605e0008aec3d5

Copy link

netlify bot commented Mar 9, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit cc03db0
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/65ec85575faafc00087537b6

@Demivan Demivan changed the title feat: preserve class names when renaming declarations feat: preserve class names when renaming declarations Mar 9, 2024
@hyf0
Copy link
Member

hyf0 commented Mar 9, 2024

Thank for your contribution.

Yes, this is an important behavior and we plan to follow the esbuild's behavior and support both function and class. It's not an easy task, since we need to integrate the feature with tree shaking and expose options to users. We don't want simply add this behavior so far.

@hyf0 hyf0 added the on-hold label Mar 9, 2024
@Demivan
Copy link
Contributor Author

Demivan commented Mar 9, 2024

Makes sense. I tried to match the Rollup implementation instead.

I will create a separate pr with c5e1779 - adds more info about failure in Rollup tests.

What can be worked on then?

@hyf0
Copy link
Member

hyf0 commented Mar 9, 2024

Makes sense. I tried to match the Rollup implementation instead.

I will create a separate pr with c5e1779 - adds more info about failure in Rollup tests.

What can be worked on then?

Glad you are interested in it. Just create a good first issue: #511

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

Successfully merging this pull request may close these issues.

None yet

2 participants