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(transformer/typescript): correctly resolve references to non-constant enum members #8543

Merged
merged 7 commits into from
Jan 18, 2025

Conversation

branchseer
Copy link
Contributor

fixes #8342

Copy link

graphite-app bot commented Jan 16, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-bug Category - Bug labels Jan 16, 2025
Copy link

codspeed-hq bot commented Jan 16, 2025

CodSpeed Performance Report

Merging #8543 will not alter performance

Comparing branchseer:non-constant-enum-member-ref (e68f8e9) with main (d3fd7a9)

Summary

✅ 32 untouched benchmarks

@overlookmotel
Copy link
Contributor

@Dunqing You wrote this transform and are more familiar with it. Would you be able to review please?

Copy link
Member

@Dunqing Dunqing left a comment

Choose a reason for hiding this comment

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

It looks great overall, we can make further improvements after #8350 is done!

crates/oxc_transformer/src/typescript/enum.rs Outdated Show resolved Hide resolved
crates/oxc_transformer/src/typescript/enum.rs Outdated Show resolved Hide resolved
@branchseer branchseer requested a review from Dunqing January 17, 2025 12:11
Copy link
Member

@Dunqing Dunqing left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@Boshen Boshen merged commit 7421a52 into oxc-project:main Jan 18, 2025
27 checks passed
@branchseer branchseer deleted the non-constant-enum-member-ref branch January 18, 2025 10:40
Boshen added a commit that referenced this pull request Jan 19, 2025
## [0.47.1] - 2025-01-19

### Features

- ee8ee55 napi/parser: Add `.hasChanged()` to `MagicString` (#8586)
(Boshen)
- 1bef911 napi/parser: Add source map API (#8584) (Boshen)

### Bug Fixes

- 7b219a9 minifier: Fix dce shadowed undefined (#8582) (Boshen)
- 7421a52 transformer/typescript: Correctly resolve references to
non-constant enum members (#8543) (branchseer)

Co-authored-by: Boshen <[email protected]>
Boshen pushed a commit that referenced this pull request Jan 19, 2025
Follow up from
#8543 (comment)

> I agree. https://github.com/oxc-project/backlog/issues/155

> Originally we were considering some form of interning and
reference-counting, so we didn't make it Copy to leave the door open for
that. But now all strings are stored in the arena anyway, so even if we
did decide to intern strings, reference-counting would be irrelevant -
our bump allocator doesn't allow freeing individual allocations anyway.

Most of the changes are done automatically by `just fix` (`cargo clippy
--fix` && `cargo fmt --all`). See the commit list for the manual edits.
Boshen pushed a commit that referenced this pull request Jan 26, 2025
…outer scope (#8691)

```typescript
var x = 10;
enum Merge { x = Math.random() }
enum Merge {
    y = x // <-- refers to Merge.x
}
```

This case wasn't covered in #8543 and by the [babel test
case](https://github.com/babel/babel/blob/e568916ef3e5fd288039a65c904b12a7eba2a73a/packages/babel-plugin-transform-typescript/test/fixtures/enum/non-constant-member-reference/input.ts).
To handle it we still have to go through the scope ancestors.

---------

Co-authored-by: Dunqing <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transformer: tsc vs babel on enum transformation
4 participants