Skip to content

Conversation

@leaysgur
Copy link
Member

@leaysgur leaysgur commented Mar 10, 2025

I happened to find this while trying out https://github.com/leaysgur/prettier-plugin-oxc .

Acorn OXC
image image

Does it make sense to add?

@graphite-app
Copy link
Contributor

graphite-app bot commented Mar 10, 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-parser Area - Parser A-ast Area - AST C-enhancement Category - New feature or request labels Mar 10, 2025
@leaysgur leaysgur marked this pull request as draft March 10, 2025 07:09
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 10, 2025

CodSpeed Performance Report

Merging #9641 will not alter performance

Comparing parser-jsx-text-raw (7de20bf) with main (f2b0cc1)

Summary

✅ 33 untouched benchmarks

@leaysgur leaysgur marked this pull request as ready for review March 10, 2025 07:23
@Boshen Boshen marked this pull request as draft March 10, 2025 13:54
@leaysgur
Copy link
Member Author

leaysgur commented Mar 11, 2025

After some research, I finally conclude that this is not as simple as just adding a property.

I will write a summary as a topic later. => #9667

@leaysgur leaysgur closed this Mar 11, 2025
@leaysgur leaysgur deleted the parser-jsx-text-raw branch March 11, 2025 02:09
@overlookmotel overlookmotel restored the parser-jsx-text-raw branch March 13, 2025 05:32
@overlookmotel overlookmotel reopened this Mar 13, 2025
@overlookmotel
Copy link
Member

overlookmotel commented Mar 13, 2025

I've re-opened as adding the raw field should be fairly simple. The huge performance hit is bizarre - this should be almost zero cost. I'll take a look and try to figure it out.

Copy link
Member


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.

@overlookmotel
Copy link
Member

overlookmotel commented Mar 13, 2025

bench

Oh no actually the perf hit is not bizarre at all. It was only on the parser_napi_raw benchmarks (which we've now disabled, so it won't show up in updated benchmarks now that I've rebased this PR on main).

String decoding is the big bottle-neck in raw transfer - it's very very slow - and this introduces lots of large strings. That's nothing to worry about - I'm going to be working on fixing string decoding perf, and then this problem will go away.

@overlookmotel
Copy link
Member

I've pushed 1 commit with a small fix. I think this is good to go now.

@overlookmotel overlookmotel marked this pull request as ready for review March 13, 2025 05:56
@overlookmotel overlookmotel requested a review from Boshen March 13, 2025 05:57
@Boshen Boshen changed the title feat(parser): Add raw property to JSXText node feat(ast)!: Add raw property to JSXText node Mar 13, 2025
@Boshen Boshen merged commit 842edd8 into main Mar 13, 2025
28 checks passed
@Boshen Boshen deleted the parser-jsx-text-raw branch March 13, 2025 06:05
@leaysgur
Copy link
Member Author

@overlookmotel Thank you for taking this over!

BTW, this code was just copied from parse_lietaral_string(), so the fixes might also be applicable to the original?

Ok(self.ast.string_literal(self.end_span(span), value, Some(raw)))

@overlookmotel
Copy link
Member

overlookmotel commented Mar 13, 2025

No thank you for your work on fixing the JS-side AST.

BTW, this code was just copied from parse_lietaral_string(), so the fixes might also be applicable to the original?

Thanks for pointing that out. #9737.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST A-parser Area - Parser C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants