Skip to content

perf(packages/codegen): check sourceText only once - #25887

Merged
graphite-app[bot] merged 1 commit into
mainfrom
om/08-18-perf_packages_codegen_check_sourcetext_only_once
Aug 19, 2026
Merged

perf(packages/codegen): check sourceText only once#25887
graphite-app[bot] merged 1 commit into
mainfrom
om/08-18-perf_packages_codegen_check_sourcetext_only_once

Conversation

@overlookmotel

@overlookmotel overlookmotel commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Follow-on after #25585. Small optimization.

Sourcemaps can't be generated unless options.sourceText is provided. This is already checked at the start. So everywhere else can then rely on state.sourceText != null, rather than re-checking it on each write.

This was referenced Aug 18, 2026
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@github-actions github-actions Bot added the A-codegen Area - Code Generation label Aug 18, 2026
@overlookmotel Graphite App

overlookmotel commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
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 changes, fast-track this PR to the front of the merge queue

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 overlookmotel mentioned this pull request Aug 18, 2026
Merged
@overlookmotel
overlookmotel requested a review from camc314 August 18, 2026 16:21
@overlookmotel
overlookmotel marked this pull request as ready for review August 18, 2026 16:21
Copilot AI lite review requested due to automatic review settings August 18, 2026 16:21
Copilot AI reviewed Aug 18, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 is a follow-on optimization to packages/codegen source map generation: it aims to validate options.sourceText once up-front when sourcemap: true, so the rest of the printing/mapping pipeline can assume state.sourceText is present and avoid repeated checks.

Changes:

  • Adds a State-constructor validation for sourceText when sourcemap is enabled, and stores sourceText as string | null.
  • Removes per-write sourceText presence checks in mapping helpers and relies on State invariants instead.
  • Updates source map generation to read sourceText from State, and adds tests for invalid/missing sourceText.

Reviewed changes

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

File Description
packages/codegen/test/source-maps.test.ts Adds tests asserting printSync rejects missing/invalid sourceText when sourcemaps are enabled.
packages/codegen/src-js/state.ts Makes State.sourceText string | null and performs sourceText validation during state construction for sourcemap builds.
packages/codegen/src-js/print/write.ts Removes repeated sourceText checks in mapping recorders and relies on State invariants.
packages/codegen/src-js/print/source_map.ts Stops reading options.sourceText, instead uses state.sourceText and always includes sourcesContent in the zero-mapping case.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/codegen/src-js/state.ts
Comment thread packages/codegen/src-js/state.ts Outdated
Comment thread packages/codegen/test/source-maps.test.ts Outdated
@overlookmotel
overlookmotel force-pushed the om/08-18-perf_packages_codegen_remove_loop_from_growmappingbuffer_ branch from 593b204 to 21612de Compare August 18, 2026 17:00
@overlookmotel
overlookmotel force-pushed the om/08-18-perf_packages_codegen_check_sourcetext_only_once branch from ea9b372 to 95e5ffe Compare August 18, 2026 17:00
@overlookmotel
overlookmotel force-pushed the om/08-18-perf_packages_codegen_remove_loop_from_growmappingbuffer_ branch from 21612de to 38701e8 Compare August 18, 2026 17:03
@overlookmotel
overlookmotel force-pushed the om/08-18-perf_packages_codegen_check_sourcetext_only_once branch from 95e5ffe to 0307c70 Compare August 18, 2026 17:03
@overlookmotel
overlookmotel requested a lite review from Copilot August 18, 2026 17:18
Copilot AI reviewed Aug 18, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@graphite-app
graphite-app Bot changed the base branch from om/08-18-perf_packages_codegen_remove_loop_from_growmappingbuffer_ to graphite-base/25887 August 18, 2026 17:25
@graphite-app
graphite-app Bot force-pushed the om/08-18-perf_packages_codegen_check_sourcetext_only_once branch from 0307c70 to d351c1e Compare August 18, 2026 17:31
@graphite-app
graphite-app Bot force-pushed the graphite-base/25887 branch from 38701e8 to 8783524 Compare August 18, 2026 17:31
@graphite-app
graphite-app Bot changed the base branch from graphite-base/25887 to main August 18, 2026 17:32
@graphite-app
graphite-app Bot force-pushed the om/08-18-perf_packages_codegen_check_sourcetext_only_once branch from d351c1e to b2f9f27 Compare August 18, 2026 17:32
camc314
camc314 reviewed Aug 18, 2026
View reviewed changes

@camc314 camc314 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we not enforce this in the type system rather than at runtime?

@overlookmotel

overlookmotel commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

can we not enforce this in the type system rather than at runtime?

I don't really see how. state.sourceText is null in non-sourcemap builds. I don't know a way to tell TypeScript "if SOURCEMAPS === true then state.sourceText !== null". Is that possible?

@camc314

camc314 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

can we not enforce this in the type system rather than at runtime?

I don't really see how. state.sourceText is null in non-sourcemap builds. I don't know a way to tell TypeScript "if SOURCEMAPS === true then state.sourceText !== null". Is that possible?

I was thinking about the logic inside the State constructor

@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Aug 19, 2026
@camc314 Graphite App

camc314 commented Aug 19, 2026 •
edited by graphite-app Bot
Loading

Copy link
Copy Markdown
Contributor

Merge activity

@overlookmotel
perf(packages/codegen): check sourceText only once (#25887)
603b904 
Follow-on after #25585. Small optimization.

Sourcemaps can't be generated unless `options.sourceText` is provided. This is already checked at the start. So everywhere else can then rely on `state.sourceText != null`, rather than re-checking it on each write.
@graphite-app
graphite-app Bot force-pushed the om/08-18-perf_packages_codegen_check_sourcetext_only_once branch from b2f9f27 to 603b904 Compare August 19, 2026 08:12
@graphite-app
graphite-app Bot merged commit 603b904 into main Aug 19, 2026
29 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Aug 19, 2026
@graphite-app
graphite-app Bot deleted the om/08-18-perf_packages_codegen_check_sourcetext_only_once branch August 19, 2026 08:16
@overlookmotel overlookmotel mentioned this pull request Aug 19, 2026
Closed
@overlookmotel

overlookmotel commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

I was thinking about the logic inside the State constructor

Ah now I get you (I think). #25916

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

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@camc314 camc314 camc314 approved these changes

Assignees

@camc314 camc314

Labels

A-codegen Area - Code Generation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@overlookmotel @camc314
Sponsor
SponsoredKunjungi sekarang
Promo