Skip to content

refactor(formatter,oxfmt): Remove EmbeddedIR and use FormatElement directly#19800

Merged
graphite-app[bot] merged 1 commit intomainfrom
02-27-refactor_formatter_oxfmt_remove_embeddedir_and_use_formatelement_directly
Feb 27, 2026
Merged

refactor(formatter,oxfmt): Remove EmbeddedIR and use FormatElement directly#19800
graphite-app[bot] merged 1 commit intomainfrom
02-27-refactor_formatter_oxfmt_remove_embeddedir_and_use_formatelement_directly

Conversation

@leaysgur
Copy link
Member

@leaysgur leaysgur commented Feb 27, 2026

Follow up on #19670 (comment)

It seems it was achievable without requiring such major changes.
Thank you for your suggestion! 🙏🏻

Copy link
Member Author


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.

@github-actions github-actions bot added A-cli Area - CLI A-formatter Area - Formatter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Feb 27, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 27, 2026

Merging this PR will not alter performance

✅ 48 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing 02-27-refactor_formatter_oxfmt_remove_embeddedir_and_use_formatelement_directly (94ac8e2) with main (9e4995c)

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@leaysgur leaysgur force-pushed the 02-27-refactor_formatter_oxfmt_remove_embeddedir_and_use_formatelement_directly branch from 0fa957f to 2b3ca30 Compare February 27, 2026 01:36
@leaysgur leaysgur changed the title refactor(formatter,oxfmt): Remove EmbeddedIR and use FormatElement directly refactor(formatter,oxfmt): Remove EmbeddedIR and use FormatElement directly Feb 27, 2026
@leaysgur leaysgur force-pushed the 02-27-refactor_formatter_oxfmt_remove_embeddedir_and_use_formatelement_directly branch 3 times, most recently from fe072d0 to 4a395e0 Compare February 27, 2026 02:32
@leaysgur leaysgur marked this pull request as ready for review February 27, 2026 02:33
@leaysgur leaysgur requested a review from Dunqing as a code owner February 27, 2026 02:33
Copilot AI review requested due to automatic review settings February 27, 2026 02:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the embedded language formatting system by removing the intermediate EmbeddedIR type and using FormatElement<'a> directly. This is a follow-up to PR #19670, which introduced GraphQL-in-JS formatting support.

Changes:

  • Removed the EmbeddedIR enum and its conversion logic from oxc_formatter
  • Modified EmbeddedDocFormatterCallback to directly return Vec<FormatElement<'a>> instead of Vec<EmbeddedIR>
  • Updated the callback to receive &Allocator and &UniqueGroupIdBuilder parameters for arena allocation and group ID management
  • Made UniqueGroupIdBuilder public to support external group ID creation
  • Moved escape_template_characters function from embed/mod.rs to from_prettier_doc.rs
  • Updated GraphQL formatting to work directly with FormatElement<'a>
  • Removed unit tests from from_prettier_doc.rs

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
crates/oxc_formatter/src/external_formatter.rs Updated EmbeddedDocFormatterCallback signature to accept allocator/group_id_builder and return FormatElement<'a> directly; removed EmbeddedIR enum definition
crates/oxc_formatter/src/lib.rs Exported additional types (Align, Condition, Group, GroupMode, Tag, TextWidth, UniqueGroupIdBuilder) needed by external callbacks
crates/oxc_formatter/src/formatter/state.rs Added group_id_builder() accessor method
crates/oxc_formatter/src/formatter/mod.rs Exported UniqueGroupIdBuilder publicly
crates/oxc_formatter/src/formatter/group_id.rs Changed UniqueGroupIdBuilder visibility from pub(super) to pub
crates/oxc_formatter/src/formatter/formatter.rs Added group_id_builder() accessor method
crates/oxc_formatter/src/formatter/format_element/tag.rs Added #[must_use] attributes to builder methods; added Align::new() constructor
crates/oxc_formatter/src/print/template/embed/mod.rs Removed write_embedded_ir() function and escape_template_characters() function (moved to from_prettier_doc.rs)
crates/oxc_formatter/src/print/template/embed/graphql.rs Updated to work directly with Vec<FormatElement<'a>> instead of Vec<EmbeddedIR>; uses allocator for text allocation
apps/oxfmt/src/prettier_compat/from_prettier_doc.rs Rewrote to convert Doc JSON directly to FormatElement<'a> instead of EmbeddedIR; added escape_template_characters() from embed/mod.rs; removed all unit tests
apps/oxfmt/src/core/external_formatter.rs Updated callback invocation to pass allocator and group_id_builder parameters

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.

Nice! I think this can also improve performance. I pushed a commit to avoid unnecessary cloning of IRs. Please also take a look at Copilot reviews before merging it

@leaysgur leaysgur force-pushed the 02-27-refactor_formatter_oxfmt_remove_embeddedir_and_use_formatelement_directly branch 2 times, most recently from 89e4fdd to 94ac8e2 Compare February 27, 2026 09:13
@leaysgur
Copy link
Member Author

Thanks as always! 🫶

I've also made it compatible with Copilot reviews, so will merge it.

@leaysgur leaysgur added the 0-merge Merge with Graphite Merge Queue label Feb 27, 2026
Copy link
Member Author

leaysgur commented Feb 27, 2026

Merge activity

…` directly (#19800)

Follow up on #19670 (comment)

It seems it was achievable without requiring such major changes.
Thank you for your suggestion! 🙏🏻
@graphite-app graphite-app bot force-pushed the 02-27-refactor_formatter_oxfmt_remove_embeddedir_and_use_formatelement_directly branch from 94ac8e2 to 805ee60 Compare February 27, 2026 09:20
@graphite-app graphite-app bot merged commit 805ee60 into main Feb 27, 2026
21 checks passed
@graphite-app graphite-app bot deleted the 02-27-refactor_formatter_oxfmt_remove_embeddedir_and_use_formatelement_directly branch February 27, 2026 09:26
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Feb 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-formatter Area - Formatter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants