Skip to content

Conversation

@gonzaloriestra
Copy link
Contributor

@gonzaloriestra gonzaloriestra commented Jul 8, 2025

WHY are these changes introduced?

Fixes https://github.com/shop/issues-develop/issues/11518

When running app dev, we need to watch for changes in imported files that live outside of the extension folders.

WHAT is this pull request doing?

  • Adds helpers in cli-kit to extract imports from JS/TS/Rust files via regex
  • When running dev, it finds imported files from the entry files of each extension and adds them to the watcher, for JS/TS/Rust files. It's run recursively, so it also finds imports from imported files.
  • Keeps a list of imported paths for each extension to know which extensions require an update
  • Refresh the paths and watcher on update events (to detect new imports without restarting)
  • It includes assets from UI extensions (should_render)
  • When watch is defined under [extensions.build], only those files are watched

How to test your changes?

  • npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]
  • Create an app with 3 extensions: JS, TS and Rust
  • For each extension, import files from a folder outside of /extensions
  • Run dev and check that it detect changes in the imported files
  • Update the imports while running dev and check it watches the expected files

Instructions to add a UI extension with assets (should_render): https://gist.github.com/elanalynn/bff3bda620cb5d0284d0cf892e713e90

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@gonzaloriestra gonzaloriestra added the stale-exempt If added, the PR/issue won't be closed by stale-bot label Jul 17, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 17, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
79.05% (+0.07% 🔼)
13462/17030
🟡 Branches
72.66% (-0.01% 🔻)
6555/9021
🟡 Functions
79.2% (+0.04% 🔼)
3469/4380
🟡 Lines
79.4% (+0.06% 🔼)
12719/16018
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / import-extractor.ts
95.45% 82.35% 100% 95.45%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / extension-instance.ts
84.58% (+1.42% 🔼)
78.51% (-0.23% 🔻)
91.94% (-0.79% 🔻)
84.86% (+1.92% 🔼)
🟢
... / app-event-watcher.ts
98.04% (+0.31% 🔼)
90.24% (-1.18% 🔻)
96.3% (+0.84% 🔼)
100%
🟡
... / file-watcher.ts
80% (-7.83% 🔻)
69.81% (-9.14% 🔻)
82.76% (-9.55% 🔻)
79.84% (-10.36% 🔻)
🟢
... / fs.ts
88.43% (+0.6% 🔼)
89.29% (-3.02% 🔻)
86% (+0.58% 🔼)
88.89% (+0.6% 🔼)
🟢
... / path.ts
89.66% (-6.9% 🔻)
80% (-10% 🔻)
93.75% (-6.25% 🔻)
92.31% (-7.69% 🔻)

Test suite run success

3280 tests passing in 1352 suites.

Report generated by 🧪jest coverage report action from 2ddec70

@gonzaloriestra gonzaloriestra marked this pull request as ready for review September 17, 2025 14:01
@gonzaloriestra gonzaloriestra requested a review from a team as a code owner September 17, 2025 14:01
@github-actions
Copy link
Contributor

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.

@gonzaloriestra
Copy link
Contributor Author

@isaacroldan Thanks for the suggestions! I've made some improvements:

  • Updated getExtensionEntryFiles to use getBundleExtensionStdinContent for UI extensions
  • Simplified the code by keeping a single scanExtensionsForImports that's used at start (with all the extensions) and on rescan (only for affected extensions)
  • Ensured that rescan and app update are called only once per change, even if the file is shared by multiple extensions

@gonzaloriestra
Copy link
Contributor Author

/snapit

@github-actions
Copy link
Contributor

🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]

Caution

After installing, validate the version by running just shopify in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@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/import-extractor.d.ts
/**
 * Extracts import paths from a source file.
 * Supports JavaScript, TypeScript, and Rust files.
 *
 * @param filePath - Path to the file to analyze.
 * @returns Array of absolute paths to imported files.
 */
export declare function extractImportPaths(filePath: string): string[];
/**
 * Recursively extracts import paths from a source file and all its dependencies.
 * Supports JavaScript, TypeScript, and Rust files.
 * Handles circular dependencies by tracking visited files.
 *
 * @param filePath - Path to the file to analyze.
 * @param visited - Set of already visited files to prevent infinite recursion.
 * @returns Array of absolute paths to the provided file and all imported files there (including nested imports).
 * @throws If an unexpected error occurs while processing files (not including ENOENT file not found errors).
 */
export declare function extractImportPathsRecursively(filePath: string, visited?: Set<string>): string[];
/**
 * Extracts import paths from a JavaScript content.
 *
 * @param content - The content to extract imports from.
 * @param filePath - The path to the file to extract imports from.
 * @returns Array of absolute paths to imported files.
 */
export declare function extractJSImports(content: string, filePath: string): string[];

Existing type declarations

packages/cli-kit/dist/public/node/fs.d.ts
@@ -155,6 +155,13 @@ export declare function mkTmpDir(): Promise<string>;
  * @returns True if the path is a directory, false otherwise.
  */
 export declare function isDirectory(path: string): Promise<boolean>;
+/**
+ * Check whether a path is a directory.
+ *
+ * @param path - Path to check.
+ * @returns True if the path is a directory, false otherwise.
+ */
+export declare function isDirectorySync(path: string): boolean;
 /**
  * Get the size of a file.
  *
@@ -273,6 +280,14 @@ export declare function generateRandomNameForSubdirectory(options: GenerateRando
  * @returns A promise that resolves to an array of pathnames that match the given pattern.
  */
 export declare function glob(pattern: Pattern | Pattern[], options?: GlobOptions): Promise<string[]>;
+/**
+ * Synchronously traverse the file system and return pathnames that match the given pattern.
+ *
+ * @param pattern - A glob pattern or an array of glob patterns.
+ * @param options - Options for the glob.
+ * @returns An array of pathnames that match the given pattern.
+ */
+export declare function globSync(pattern: Pattern | Pattern[], options?: GlobOptions): string[];
 /**
  * Convert a path to a File URL.
  *

Copy link
Contributor

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

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

Great job on this 👏

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Oct 17, 2025
Merged via the queue into main with commit cb4d69b Oct 17, 2025
2 checks passed
@gonzaloriestra gonzaloriestra deleted the watch-imports branch October 17, 2025 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-exempt If added, the PR/issue won't be closed by stale-bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants