feat(transformer/react): correct import binding names

[Dunqing]

No description provided.

[Dunqing]

• feat(transformer/react): correct import binding names #3014 👈
• main

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

Join @Dunqing and the rest of your teammates on Graphite

[Dunqing]

We need to collect all bindings first.

[codspeed-hq]

CodSpeed Performance Report

Merging #3014 will degrade performances by 4.13%

_{Comparing 04-17-feat_transformer_react_correct_import_binding_names (9660992) with main (5d89e75)}

Summary

❌ 3 regressions
✅ 27 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	04-17-feat_transformer_react_correct_import_binding_names	Change
❌	transformer[RadixUIAdoptionSection.jsx]	922.1 µs	961.9 µs	-4.13%
❌	transformer[antd.js]	1.5 s	1.6 s	-3.62%
❌	transformer[cal.com.tsx]	376.9 ms	389.4 ms	-3.2%

[Dunqing]

TransformerNaming needs to mutation.

In a previous implementation. All the TransformerCtx fields that cannot be copied are wrapped in Rc

oxc/crates/oxc_transformer/src/context.rs

Lines 15 to 22 in 31ed532

	#[derive(Clone)]
	pub struct TransformerCtx<'a> {
	pub ast: Rc<AstBuilder<'a>>,
	pub options: Cow<'a, TransformOptions>,
	semantic: Rc<RefCell<Semantic<'a>>>,
	errors: Rc<RefCell<Vec<Error>>>,
	}

[milesj]

This is a big change.

It probably makes more sense to use interior mutability in TransformerNaming, instead of adding all these borrow calls everywhere.

[Dunqing]

It probably makes more sense to use interior mutability in TransformerNaming, instead of adding all these borrow calls everywhere.

Yes, I agree. When we finally consider this PR, I'll refactor it.

[Boshen]

Let me iterate on this tomorrow.

[Boshen]

I'll find sometime to figure out recreating scopes and bindings during visits, probably this weekend.

[milesj]

I'll find sometime to figure out recreating scopes and bindings during visits, probably this weekend.

The problem we had before is that the nodes don't have IDs, so there was no easy way to back reference. That's why @rzvxa was trying to add IDs to everything.

[rzvxa]

I'll find sometime to figure out recreating scopes and bindings during visits, probably this weekend.

The problem we had before is that the nodes don't have IDs, so there was no easy way to back reference. That's why @rzvxa was trying to add IDs to everything.

We still can add this, I didn't receive any pushback while implementing it in #2876 and #2818, I just stopped to figure out the traverse first; Now that the traverse is almost sorted out we may want to revisit the ast_node_id.
Since now we have a parent field for all nodes because of the traverse, We can change the parent with an AstPath structure containing both the opaque parent's pointer and our own ID.

[rzvxa]

I suspect that by adding node_ids to the nodes we can do a minimal refactor for linters which removes a bunch of useless checks and improves the performance by like 10% to 20%.

We can still provide a way to handle AstNodes so we don't have to refactor them all from day one, But we can offer additional traits to intercept the inner node without needing extra match on AstKind.

---

Edit:

The whole reason behind AstNode is having semantic information along with our nodes.

[rzvxa]

I'm just thinking this one out loud;

Since now we have a parent pointer - even though it is opaque - we might be able to map between that and our own ast_node_id so we don't have to add an extra usize to our nodes.

For example if we always allocate all of our nodes in the same buffer and we have the root of our buffer we can find our relative position to that and use it to fetch our relative data in a different buffer similar to an indexing operation.