Skip to content

Introduce TomlFile abstraction for TOML file I/O#6943

Merged
ryancbahan merged 4 commits intomainfrom
rcb/toml-file-class
Mar 11, 2026
Merged

Introduce TomlFile abstraction for TOML file I/O#6943
ryancbahan merged 4 commits intomainfrom
rcb/toml-file-class

Conversation

@ryancbahan
Copy link
Contributor

@ryancbahan ryancbahan commented Mar 6, 2026

Summary

Adds a general-purpose TomlFile class in cli-kit with read/patch/remove/replace/transformRaw methods. Restructures TOML utilities into a toml/ directory with internal codec module (encodeToml/decodeToml).

Why this change exists

Today, the concerns of "parse a file", "validate a file", "validate Shopify domain config", and "transform file contents to domain config" are interwoven and dispersed across callsites. This makes it very difficult to instrument changes or drive consistency in the codebase. This pr introduces a TOML file abstraction that owns a specific slice of jobs: read/write TOML, and check whether that TOML is valid syntax. It does not care about Shopify domain knowledge or validation. This is the first step in providing a clearer lifecycle of config read/writes, and separation between transform/validate/IO jobs.

Current problems

  • Each callsite making its own choices about error handling, comment preservation, and I/O.
  • Serialization and I/O mixed across file boundaries. Extension builders called encodeToml() and returned TOML strings. Their caller (import-extensions.ts) called writeFile() to put those strings on disk.
    The responsibility for "write a TOML file" is split between the function that knew the content and the function that knew the path.
  • Multiple approaches to reads/writes: some callsites can preserve comments, others use encode/decode functions directly and cannot.
  • Parse errors lost context. decodeToml throws raw @iarna/toml errors with line/column but no file path. Each callsite had to catch and re-wrap with path context independently — or didn't, producing confusing errors.

With this PR:

  • One abstraction for all TOML file operations. read/patch/remove/replace/transformRaw. Every TOML file operation in the codebase goes through it.
  • Comment preservation is built into the model. patch and remove use toml_edit (via WASM) to preserve comments and formatting. replace + transformRaw handles the comment injection case for config link. Subsequent edits via dev/deploy/urls preserve whatever comments exist. This was previously implicit knowledge spread across callsites.
  • Parse errors include file path automatically. TomlParseError wraps parse failures with the file path. Every consumer of TomlFile.read() gets good error messages without extra catch/re-throw boilerplate.
  • encodeToml/decodeToml become internal. The public API surface shrinks. Consumers use TomlFile for file I/O. The codec functions are implementation details in cli-kit/node/toml/codec.ts, not something
    every file in the app package imports.
  • Foundation for schema validation; beginning of separation between file, validation, and transform concerns.

Test plan

  • 20 TomlFile unit tests (read/patch/remove/replace/transformRaw + error handling)
  • All existing cli-kit tests pass

@ryancbahan ryancbahan changed the title Introduce TomlFile abstraction for TOML file I/O 1/3: Introduce TomlFile abstraction for TOML file I/O Mar 6, 2026
@ryancbahan ryancbahan force-pushed the rcb/toml-file-class branch from 31f057f to 80bf13d Compare March 6, 2026 17:45
@ryancbahan ryancbahan changed the title 1/3: Introduce TomlFile abstraction for TOML file I/O Introduce TomlFile abstraction for TOML file I/O Mar 6, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 78.87% 14518/18408
🟡 Branches 73.16% 7209/9854
🟡 Functions 79.09% 3703/4682
🟡 Lines 79.21% 13717/17317

Test suite run success

3812 tests passing in 1456 suites.

Report generated by 🧪jest coverage report action from a4d144a

@ryancbahan ryancbahan marked this pull request as ready for review March 6, 2026 18:03
@ryancbahan ryancbahan requested a review from a team as a code owner March 6, 2026 18:03
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

@binks-code-reviewer
Copy link

binks-code-reviewer bot commented Mar 6, 2026

🤖 Code Review · #projects-dev-ai for questions
React with 👍/👎 or reply — all feedback helps improve the agent.

Complete - No issues

📋 History

✅ 1 findings → ✅ No issues → ✅ 1 findings → ✅ No issues → ✅ No issues

@phyllis-sy-wu
Copy link
Contributor

Tophatted, everything looked good with no errors or weird behaviour

Scenarios tested (in cli repo: pnpm shopify app xxxxx --path ):

  • App without extensions → app dev → app deploy ✅
  • App with extensions (checkout-ui, product-configuration, theme-extension, discount-function) → app dev → app deploy ✅
  • Compared with prod CLI (shopify app )

Result:

  • Behaviour aligned with apps created/developed from prod CLI
  • Config was read and updated as expected
  • TOML stayed valid and comments were preserved
  • Deploy completed with no validation errors (payload accepted).

"./node/toml": {
"node": "./dist/public/node/toml/index.js",
"types": "./dist/public/node/toml/index.d.ts"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

you shouldn't need this, is already covered by the wildcard below :thinking_face:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, IIRC imports from the new toml dir didn't work without this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea this is necessary to import from `@shopify/cli-kit/node/toml which the next pr does

Copy link
Contributor

Choose a reason for hiding this comment

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

all other modules work with the wildcard export, for instance /node/crypto or /node/path
I guess the issue is having an index file, can we do this without an index file and expose a toml.ts directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

node/crypto and node/path both point to files in the public/node root, not subfolders. i put the toml files in their own subdir since it's a domain with multiple file instances and i didn't want to keep expanding the root, which is already very large.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i guess we could do re-exports from the root, but i don't see much value in it

Copy link
Contributor

Choose a reason for hiding this comment

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

Just worried of having to manually export things here, can grow a lot very quickly, let's leave it for now and we can revisit if this grows too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just curious -- why? The cost of change is basically zero to update an import alias/etc. I personally prefer more structured directories, as it's easier to visually navigate.

Copy link
Contributor

Choose a reason for hiding this comment

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

just a preference of having automatic exports (or maybe an index file) and not needing to update the package.json for every folder/file we add to the project

@ryancbahan ryancbahan requested a review from isaacroldan March 9, 2026 16:43
const raw = await readFile(this.path)
const updated = updateTomlValues(raw, [[keys, undefined]])
await writeFile(this.path, updated)
this.content = this.decode(updated)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we decode before writing to disk to ensure validity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this and didn't because we're effectively testing @shopify/toml-patch for that. but on reflection I think the hardening is better in any case. updating

@ryancbahan ryancbahan requested a review from dmerand March 9, 2026 17:09
ryancbahan and others added 4 commits March 11, 2026 14:08
Adds a general-purpose TomlFile class in cli-kit with read/patch/remove/
replace/transformRaw methods. Restructures toml utilities into a toml/
directory with codec (internal encode/decode) and index (public JsonMapType
export) modules. Only TomlFile and JsonMapType are publicly exported.

No callsite migrations in this commit — purely additive.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ryancbahan ryancbahan force-pushed the rcb/toml-file-class branch from e13bc32 to a4d144a Compare March 11, 2026 14:08
@github-actions
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

packages/cli-kit/dist/public/node/toml/codec.d.ts
import { JsonMap } from '../../../private/common/json.js';
export type JsonMapType = JsonMap;
/**
 * Given a TOML string, it returns a JSON object.
 *
 * @param input - TOML string.
 * @returns JSON object.
 */
export declare function decodeToml(input: string): JsonMapType;
/**
 * Given a JSON object, it returns a TOML string.
 *
 * @param content - JSON object.
 * @returns TOML string.
 */
export declare function encodeToml(content: JsonMap | object): string;
packages/cli-kit/dist/public/node/toml/index.d.ts
export { decodeToml, encodeToml } from './codec.js';
export type { JsonMapType } from './codec.js';
packages/cli-kit/dist/public/node/toml/toml-file.d.ts
import { JsonMapType } from './codec.js';
/**
 * Thrown when a TOML file cannot be parsed. Includes the file path for context.
 */
export declare class TomlParseError extends Error {
    readonly path: string;
    constructor(path: string, cause: Error);
}
/**
 * General-purpose TOML file abstraction.
 *
 * Provides a unified interface for reading, patching, removing keys from, and replacing
 * the content of TOML files on disk.
 *
 * - `read` populates content from disk
 * - `patch` does surgical WASM-based edits (preserves comments and formatting)
 * - `remove` deletes a key by dotted path (preserves comments and formatting)
 * - `replace` does a full re-serialization (comments and formatting are NOT preserved).
 * - `transformRaw` applies a function to the raw TOML string on disk.
 */
export declare class TomlFile {
    /**
     * Read and parse a TOML file from disk. Throws if the file doesn't exist or contains invalid TOML.
     * Parse errors are wrapped in {@link TomlParseError} with the file path for context.
     *
     * @param path - Absolute path to the TOML file.
     * @returns A TomlFile instance with parsed content.
     */
    static read(path: string): Promise<TomlFile>;
    readonly path: string;
    content: JsonMapType;
    constructor(path: string, content: JsonMapType);
    /**
     * Surgically patch values in the TOML file, preserving comments and formatting.
     *
     * Accepts a nested object whose leaf values are set in the TOML. Intermediate tables are
     * created automatically. Setting a leaf to `undefined` removes it (use `remove()` for a
     * clearer API when deleting keys).
     *
     * @example
     * ```ts
     * await file.patch({build: {dev_store_url: 'my-store.myshopify.com'}})
     * await file.patch({application_url: 'https://example.com', auth: {redirect_urls: ['...']}})
     * ```
     */
    patch(changes: {
        [key: string]: unknown;
    }): Promise<void>;
    /**
     * Remove a key from the TOML file by dotted path, preserving comments and formatting.
     *
     * @param keyPath - Dotted key path to remove (e.g. 'build.include_config_on_deploy').
     * @example
     * ```ts
     * await file.remove('build.include_config_on_deploy')
     * ```
     */
    remove(keyPath: string): Promise<void>;
    /**
     * Replace the entire file content. The file is fully re-serialized — comments and formatting
     * are NOT preserved.
     *
     * @param content - The new content to write.
     * @example
     * ```ts
     * await file.replace({client_id: 'abc', name: 'My App'})
     * ```
     */
    replace(content: JsonMapType): Promise<void>;
    /**
     * Transform the raw TOML string on disk. Reads the file, applies the transform function
     * to the raw text, writes back, and re-parses to keep `content` in sync.
     *
     * Use this for text-level operations that can't be expressed as structured edits —
     * e.g. Injecting comments or positional insertion of keys in arrays-of-tables.
     * Subsequent `patch()` calls will preserve any comments added this way.
     *
     * @param transform - A function that receives the raw TOML string and returns the modified string.
     * @example
     * ```ts
     * await file.transformRaw((raw) => `# Header comment\n${raw}`)
     * ```
     */
    transformRaw(transform: (raw: string) => string): Promise<void>;
    private decode;
}

Existing type declarations

We found no diffs with existing type declarations

@ryancbahan ryancbahan added this pull request to the merge queue Mar 11, 2026
Merged via the queue into main with commit d2ffab5 Mar 11, 2026
25 checks passed
@ryancbahan ryancbahan deleted the rcb/toml-file-class branch March 11, 2026 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants