fix(transformer/legacy-decorator): metadata should be inserted after all params decorators

[Dunqing]

• Fixes transformer: param decorator should be executed before metadata injections #13210

[Dunqing]

• fix(transformer/legacy-decorator): metadata should be inserted after all params decorators #13215 👈 (View in Graphite)
• main

---

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.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[Copilot]

Pull Request Overview

This PR fixes the order of decorators in the legacy decorator transformer to ensure metadata decorators are inserted after parameter decorators. The issue was that metadata decorators (specifically decorateMetadata) were being inserted before parameter decorators (decorateParam), which is incorrect according to the decorator specification.

• Reorders decorator insertion to place parameter decorators before metadata decorators
• Updates test fixtures to reflect the correct decorator ordering
• Improves test coverage by adding a new test case for parameter decorators

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/oxc_transformer/src/decorator/legacy/mod.rs	Core fix that splits and reorders decorators to ensure metadata comes after parameters
tasks/transform_conformance/tests/legacy-decorators/test/fixtures/oxc/metadata/params/input.ts	New test case with various parameter decorator scenarios
tasks/transform_conformance/tests/legacy-decorators/test/fixtures/oxc/metadata/params/output.js	Expected output for the new parameter decorator test
Multiple output.js/.ts files	Updated expected outputs reflecting the corrected decorator ordering
tasks/transform_conformance/snapshots/oxc.snap.md	Updated test results showing one additional passing test

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #13215 will not alter performance

_{Comparing 08-19-fix_transformer_legacy-decorator_metadata_should_be_inserted_after_all_params_decorators (76a9865) with main (8264664)¹}

Summary

✅ 34 untouched benchmarks

Footnotes

1. No successful run was found on main (76a9865) during the generation of this report, so 8264664 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[overlookmotel]

My only question, which isn't actually relevant to this PR specifically, is why metadata decorators are added to method.decorators, only to be removed again.

The method of distinguishing "real" decorators from metadata decorators by whether they have an empty span works now because no other transform creates new unspanned "real" decorators. But it's not so future-proof.

1. It couples this transform to all the other transforms via this assumption.
2. Once we have transform plugins, user code might create new decorators with empty spans, and then this code would malfunction.

An alternative which I think could work: Transform hold a stack of Vecs, each representing "decorators to be added to current class". As it traverses through methods, props etc, it'd add transpiled decorators to that Vec. Then exit_class would add them all after the class.

Maybe that's also more efficient, as it doesn't require looping through all class elements again in exit_class. Maybe...

[overlookmotel]

Merge activity

• Aug 19, 4:08 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Aug 19, 4:08 PM UTC: overlookmotel added this pull request to the Graphite merge queue.
• Aug 19, 4:14 PM UTC: Merged by the Graphite merge queue.

[Dunqing]

My only question, which isn't actually relevant to this PR specifically, is why metadata decorators are added to method.decorators, only to be removed again.

The method of distinguishing "real" decorators from metadata decorators by whether they have an empty span works now because no other transform creates new unspanned "real" decorators. But it's not so future-proof.

1. It couples this transform to all the other transforms via this assumption.
2. Once we have transform plugins, user code might create new decorators with empty spans, and then this code would malfunction.

Thanks for reviewing this. I agree with your point, which is what I am trying to avoid, but it is troublesome in practice.

An alternative which I think could work: Transform hold a stack of Vecs, each representing "decorators to be added to current class". As it traverses through methods, props etc, it'd add transpiled decorators to that Vec. Then exit_class would add them all after the class.

Maybe that's also more efficient, as it doesn't require looping through all class elements again in exit_class. Maybe...

I'm not sure if I understand what you mean. Let me describe the transformation order here

Currently, metadata decorators are inserted during the enter_* phase, and decorators are transformed in the exit_class, which is unchangeable because we have to collect metadata before the TypesScript eliminates the type annotations, and the reason that we transform the decorators in exit_class is the decoratos may contains some syntax that should be transpiled by other plugins first.

In your approach, I don't think the problem that metadata is being inserted before the real param decorators are inserted is solved.

I had a thought that we could store metadata in a temporary Vec, and take it when transforming decorators. The solution is similar to your approach of "holding a stack of Vec", but the problem is that we have to consider the nested classes case, and it may affect performance because it would produce many temporary Vec. I am not seeing that it is better than the current.

To sum up, I am happy to make changes for this. The reason I use span.is_unspanned to identify metadata because this is the easiest way to fix the problem as quickly as possible.