oxc icon indicating copy to clipboard operation
oxc copied to clipboard

feat(transformer/react): correct import binding names

Open Dunqing opened this issue 10 months ago • 8 comments

Dunqing avatar Apr 17 '24 11:04 Dunqing

  • #3014 Graphite 👈
  • main

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

Join @Dunqing and the rest of your teammates on Graphite Graphite

Dunqing avatar Apr 17 '24 11:04 Dunqing

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

:warning: 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%

codspeed-hq[bot] avatar Apr 17 '24 11:04 codspeed-hq[bot]

Let me iterate on this tomorrow.

Boshen avatar Apr 17 '24 14:04 Boshen

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

Boshen avatar Apr 18 '24 06:04 Boshen

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.

milesj avatar Apr 18 '24 16:04 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.

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 avatar Apr 19 '24 07:04 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 avatar Apr 19 '24 07:04 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.

rzvxa avatar Apr 19 '24 07:04 rzvxa

Finally, we have a generateUid api. #3394

Dunqing avatar May 24 '24 08:05 Dunqing