Skip to content

Migrate all TOML I/O callsites to TomlFile#6944

Merged
ryancbahan merged 1 commit intomainfrom
rcb/toml-migrate-callsites
Mar 11, 2026
Merged

Migrate all TOML I/O callsites to TomlFile#6944
ryancbahan merged 1 commit intomainfrom
rcb/toml-migrate-callsites

Conversation

@ryancbahan
Copy link
Contributor

@ryancbahan ryancbahan commented Mar 6, 2026

Summary

Migrates every TOML file I/O callsite to use TomlFile:

  • dev.ts, context.ts, urls.ts: setAppConfigValue/unsetAppConfigValueTomlFile.patch()/remove()
  • write-app-configuration-file.ts: encodeToml + writeFileSyncTomlFile.replace() + transformRaw()
  • add-uid-to-extension-toml.ts: raw readFile/writeFileTomlFile.read() + patch()/transformRaw()
  • breakdown-extensions.ts: encodeToml → regex → field names replaced with Object.keys() on diff objects
  • Extension builders: return objects instead of TOML strings
  • loader.ts: removed unused decode parameter from loadConfigurationFileContent/parseConfigurationFile
  • environments.ts: decodeTomlTomlFile.read()
  • patch-app-configuration-file.ts: removed all TOML functions (only patchAppHiddenConfigFile remains)

encodeToml/decodeToml are no longer imported outside cli-kit.

Test plan

  • All 1688 app tests pass (services + models)
  • All cli-kit tests pass
  • TypeScript type checks pass for both packages

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 78.86% 14504/18391
🟡 Branches 73.18% 7198/9836
🟡 Functions 79.06% 3697/4676
🟡 Lines 79.19% 13704/17305

Test suite run success

3802 tests passing in 1453 suites.

Report generated by 🧪jest coverage report action from 80d65bc

@ryancbahan ryancbahan changed the title Migrate all TOML I/O callsites to TomlFile 2/3: Migrate all TOML I/O callsites to TomlFile Mar 6, 2026
@ryancbahan ryancbahan force-pushed the rcb/toml-migrate-callsites branch from f9b265f to 7e9554f Compare March 6, 2026 17:45
@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 2/3: Migrate all TOML I/O callsites to TomlFile Migrate all TOML I/O callsites to TomlFile Mar 6, 2026
@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.

const regex = /\n?((\s*)type\s*=\s*"\S*")/
updatedTomlContents = tomlContents.replace(regex, `$2\nuid = "${extension.uid}"\n$1`)
// Single extension (or no extensions array): add uid at the top level via WASM patch
await file.patch({uid: extension.uid})

Choose a reason for hiding this comment

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

Single-extension path may write uid at top-level instead of inside [[extensions]]

Previously, the single-extension branch inserted uid before the first type = ... occurrence, which typically lives inside [[extensions]]. Now it does:

await file.patch({uid: extension.uid})

This patches uid at the top level rather than inside extensions[0]. Meanwhile, the multi-extension branch inserts uid next to the matching handle inside a specific [[extensions]] table, yielding inconsistent output shape depending on file structure. If downstream expects per-extension [[extensions]].uid, a top-level uid may be ignored or misinterpreted. Additionally, if ('uid' in file.content) return now treats a top-level uid as completed even if per-extension uid is missing.

@binks-code-reviewer
Copy link

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

Complete - 2 findings

📋 History

✅ 2 findings

}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return encodeToml(localExtensionRepresentation as any)
return localExtensionRepresentation
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, so then buildTomlObject doesn't build a toml object anymore no? it builds a JSON that might or might not be converted to toml. Should this buildTomlObject return a TomlFile? should we rename it?

Copy link
Contributor

@isaacroldan isaacroldan Mar 9, 2026

Choose a reason for hiding this comment

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

same for all the other buildTomlObject that exist in other files, seems like these are the main changes in this PR (by number of lines), should this function just use TomlFile and in the tests, instead of validating the JSON, validate the TomlFile raw content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nex pr in the stack renames the functions to indicate that it's not specifically toml - I just didn't want to include here as the function reference change would make this pr more indirect. I went with a name change instead of consuming TOML directly but am happy to talk about it. I think it makes sense to pass around object reference and convert to/from toml in narrower pipelines.

@ryancbahan ryancbahan requested a review from isaacroldan March 9, 2026 15:38
@ryancbahan ryancbahan force-pushed the rcb/toml-migrate-callsites branch from 7e9554f to 509a528 Compare March 9, 2026 15:38
@ryancbahan ryancbahan requested a review from dmerand March 9, 2026 16:16
@ryancbahan ryancbahan force-pushed the rcb/toml-migrate-callsites branch 2 times, most recently from 82055c8 to 7749fae Compare March 9, 2026 17:08
Copy link
Contributor

@dmerand dmerand left a comment

Choose a reason for hiding this comment

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

This is a great improvement! I left some notes from AI pair review, but they're not blocking.


Review assisted by pair-review

Copy link
Contributor

Choose a reason for hiding this comment

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

🧑‍🍳 💋 to how much simpler this is.

{keyPath: 'application_url', value: urls.applicationUrl},
{keyPath: 'auth.redirect_urls', value: urls.redirectUrlWhitelist},
]
const configFile = await TomlFile.read(localApp.configuration.path)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Improvement: The current implementation builds the patch object with conditional property assignment—creating a mutable patch variable that's modified across different code paths. While clear, this could be more concise using object spread to make the conditional inclusion of app_proxy more explicit and avoid mutating state.

Suggestion: Consider using a single object spread:

const configFile = await TomlFile.read(localApp.configuration.path)
await configFile.patch({
  application_url: urls.applicationUrl,
  auth: {redirect_urls: urls.redirectUrlWhitelist},
  ...(urls.appProxy && {
    app_proxy: {
      url: urls.appProxy.proxyUrl,
      subpath: urls.appProxy.proxySubPath,
      prefix: urls.appProxy.proxySubPathPrefix,
    },
  }),
})

This makes it clearer that app_proxy is conditionally included and avoids mutating the patch object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, there's improvement to be done here. trying to mirror pre-existing behavior 1:1 first and then keep addressing this stuff, rather than mixing concerns.

await writeFile(path, tomlObject)
const tomlContent = buildTomlObject(ext, extensions, app.configuration)
const tomlPath = joinPath(directory, 'shopify.extension.toml')
const file = new TomlFile(tomlPath, tomlContent as JsonMapType)
Copy link
Contributor

Choose a reason for hiding this comment

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

🐛 Bug: Line 116 creates a TomlFile with tomlContent as the second argument, but line 117 immediately calls replace() with the same content. The TomlFile constructor sets this.content = content, so passing tomlContent to the constructor is redundant since it's overwritten by replace(). This pattern differs from other parts of the codebase where TomlFile is initialized with an empty object before replacement.

Suggestion:

Suggested change
const file = new TomlFile(tomlPath, tomlContent as JsonMapType)
const file = new TomlFile(tomlPath, {})

@ryancbahan ryancbahan force-pushed the rcb/toml-file-class branch from e13bc32 to a4d144a Compare March 11, 2026 14:08
Replaces scattered setAppConfigValue/setManyAppConfigValues/unsetAppConfigValue
with TomlFile.patch/remove. Extension builders return objects instead of TOML
strings. writeAppConfigurationFile uses TomlFile.replace + transformRaw for
comment injection. breakdown-extensions uses Object.keys() instead of
encode-then-regex-parse. Removes decode parameter from loadConfigurationFileContent.
encodeToml/decodeToml no longer imported outside cli-kit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ryancbahan ryancbahan force-pushed the rcb/toml-migrate-callsites branch from 7749fae to 80d65bc 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 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

Base automatically changed from rcb/toml-file-class to main March 11, 2026 14:54
@ryancbahan ryancbahan added this pull request to the merge queue Mar 11, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 11, 2026
@ryancbahan ryancbahan added this pull request to the merge queue Mar 11, 2026
Merged via the queue into main with commit a1e2462 Mar 11, 2026
23 checks passed
@ryancbahan ryancbahan deleted the rcb/toml-migrate-callsites branch March 11, 2026 15:14
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.

3 participants