Better path handling for TurboSnap

bin/git/git.js

@@ -99,7 +99,7 @@ export async function getSlug() {
 
 // We could cache this, but it's probably pretty quick
 export async function getCommit() {
-  const result = await execGitCommand(`git log -n 1 --format="%H ## %ct ## %ce ## %cn"`);
+  const result = await execGitCommand(`git --no-pager log -n 1 --format="%H ## %ct ## %ce ## %cn"`);
   const [commit, committedAtSeconds, committerEmail, committerName] = result.split(' ## ');
   return { commit, committedAt: committedAtSeconds * 1000, committerEmail, committerName };
 }
@@ -126,7 +126,7 @@ export async function getBranch() {
 }
 
 export async function hasPreviousCommit() {
-  const result = await execGitCommand(`git log -n 1 --skip=1 --format="%H"`);
+  const result = await execGitCommand(`git --no-pager log -n 1 --skip=1 --format="%H"`);
   return !!result.trim();
 }
 
@@ -334,7 +334,7 @@ export async function getBaselineBuilds({ client }, { branch, parentCommits }) {
 
 export async function getChangedFiles(baseCommit, headCommit = '') {
   // Note that an empty headCommit will include uncommitted (staged or unstaged) changes.
-  const files = await execGitCommand(`git diff --name-only ${baseCommit} ${headCommit}`);
+  const files = await execGitCommand(`git --no-pager diff --name-only ${baseCommit} ${headCommit}`);
   return files.split(EOL).filter(Boolean);
 }
 

bin/lib/getDependentStoryFiles.js

@@ -11,29 +11,43 @@ const GLOBALS = [/\/node_modules\//, /\/package\.json$/, /\/package-lock\.json$/
 const EXTERNALS = [/\/node_modules\//, /\/webpack\/runtime\//, /^\(webpack\)/];
 
 const isGlobal = (name) => GLOBALS.some((re) => re.test(name));
-const isUserCode = ({ name, moduleName }) => !EXTERNALS.some((re) => re.test(name || moduleName));
+const isUserModule = ({ id, name, moduleName }) =>
+  id !== undefined && id !== null && !EXTERNALS.some((re) => re.test(name || moduleName));
 
 // Replaces Windows-style backslash path separators with POSIX-style forward slashes, because the
 // Webpack stats use forward slashes in the `name` and `moduleName` fields. Note `changedFiles`
 // already contains forward slashes, because that's what git yields even on Windows.
 const posix = (localPath) => localPath.split(path.sep).filter(Boolean).join(path.posix.sep);
-const clean = (posixPath) => (posixPath.startsWith('./') ? posixPath.slice(2) : posixPath);
 
+/**
+ * Converts a module path found in the webpack stats to be relative to the (git) root path. Module
+ * paths can be relative (`./module.js`) or absolute (`/path/to/project/module.js`). The webpack
+ * stats may have been generated in a subdirectory, so we prepend the workingDir if necessary.
+ * The result is a relative POSIX path compatible with `git diff --name-only`.
+ */
+export function normalizePath(posixPath, rootPath, workingDir = '') {
+  if (!posixPath) return posixPath;
+  return path.posix.isAbsolute(posixPath)
+    ? path.posix.relative(rootPath, posixPath)
+    : path.posix.join(workingDir, posixPath);
+}
+
+/**
+ * This traverses the webpack module stats to retrieve a set of CSF files that somehow trace back to
+ * the changed git files. The result is a map of Module ID => file path. In the end we'll only send
+ * the Module IDs to Chromatic, the file paths are only for logging purposes.
+ */
 export async function getDependentStoryFiles(ctx, stats, changedFiles) {
   const { configDir = '.storybook', staticDir = [] } = ctx.storybook || {};
 
+  // Currently we enforce Storybook to be built by the Chromatic CLI, to ensure absolute paths match
+  // up between the webpack stats and the git repo root.
   const rootPath = await getRepositoryRoot(); // e.g. `/path/to/project` (always absolute posix)
-  const workingDir = posix(getWorkingDir(rootPath)); // e.g. `services/webapp` or empty string
-  const workingDirRegExp = workingDir && new RegExp(`^./${workingDir}`);
+  const workingDir = getWorkingDir(rootPath); // e.g. `packages/storybook` or empty string
+  const normalize = (posixPath) => normalizePath(posixPath, rootPath, workingDir);
 
-  // Module paths can be relative (`./module.js`) or absolute (`/path/to/project/services/webapp/module.js`)
-  // By stripping either prefix, we have a consistent format to compare against (i.e. `module.js`).
-  const basePath = path.posix.join(rootPath, workingDir);
-  const baseRegExp = new RegExp(`^./|${basePath}/`);
-  const base = (posixPath) => posixPath && posixPath.replace(baseRegExp, '');
-
-  const storybookDir = base(posix(configDir));
-  const staticDirs = staticDir.map((dir) => base(posix(dir)));
+  const storybookDir = normalize(posix(configDir));
+  const staticDirs = staticDir.map((dir) => normalize(posix(dir)));
 
   // NOTE: this only works with `main:stories` -- if stories are imported from files in `.storybook/preview.js`
   // we'll need a different approach to figure out CSF files (maybe the user should pass a glob?).
@@ -43,46 +57,46 @@ export async function getDependentStoryFiles(ctx, stats, changedFiles) {
   const reasonsById = {};
   const csfGlobsByName = {};
 
-  stats.modules.filter(isUserCode).forEach((mod) => {
-    const baseModuleName = base(mod.name);
+  stats.modules.filter(isUserModule).forEach((mod) => {
+    const normalizedName = normalize(mod.name);
+    idsByName[normalizedName] = mod.id;
 
-    if (mod.id) {
-      idsByName[baseModuleName] = mod.id;
-      (mod.modules ? mod.modules.map((m) => base(m.name)) : []).forEach((baseName) => {
-        idsByName[baseName] = mod.id;
+    if (mod.modules) {
+      mod.modules.forEach((m) => {
+        idsByName[normalize(m.name)] = mod.id;
       });
     }
 
     reasonsById[mod.id] = mod.reasons
-      .map((reason) => base(reason.moduleName))
-      .filter((reasonName) => reasonName && reasonName !== baseModuleName);
+      .map((reason) => normalize(reason.moduleName))
+      .filter((reasonName) => reasonName && reasonName !== normalizedName);
 
     if (reasonsById[mod.id].includes(storiesEntryFile)) {
-      csfGlobsByName[baseModuleName] = true;
+      csfGlobsByName[normalizedName] = true;
     }
   });
 
   ctx.log.debug(`Found ${Object.keys(csfGlobsByName).length} CSF globs`);
   ctx.log.debug(`Found ${Object.keys(idsByName).length} user modules`);
 
   const isCsfGlob = (name) => !!csfGlobsByName[name];
-  const isConfigFile = (name) => name.startsWith(storybookDir) && name !== storiesEntryFile;
-  const isStaticFile = (name) => staticDirs.some((dir) => name.startsWith(dir));
+  const isConfigFile = (name) => name.startsWith(`${storybookDir}/`) && name !== storiesEntryFile;
+  const isStaticFile = (name) => staticDirs.some((dir) => name.startsWith(`${dir}/`));
 
   const changedCsfIds = new Set();
   const checkedIds = {};
   const toCheck = [];
 
   let bail = changedFiles.find(isGlobal);
 
-  function traceName(name) {
-    if (bail || isCsfGlob(name)) return;
-    if (isConfigFile(name) || isStaticFile(name)) {
-      bail = name;
+  function traceName(normalizedName) {
+    if (bail || isCsfGlob(normalizedName)) return;
+    if (isConfigFile(normalizedName) || isStaticFile(normalizedName)) {
+      bail = normalizedName;
       return;
     }
 
-    const id = idsByName[name];
+    const id = idsByName[normalizedName];
     if (!id || !reasonsById[id] || checkedIds[id]) return;
     toCheck.push(id);
 
@@ -91,10 +105,7 @@ export async function getDependentStoryFiles(ctx, stats, changedFiles) {
     }
   }
 
-  changedFiles.forEach((gitFilePath) => {
-    const webpackFilePath = workingDir ? gitFilePath.replace(workingDirRegExp, '.') : gitFilePath;
-    traceName(clean(webpackFilePath));
-  });
+  changedFiles.forEach(traceName);
   while (toCheck.length > 0) {
     const id = toCheck.pop();
     checkedIds[id] = true;
@@ -108,7 +119,7 @@ export async function getDependentStoryFiles(ctx, stats, changedFiles) {
 
   return stats.modules.reduce((acc, mod) => {
     if (changedCsfIds.has(mod.id))
-      acc[String(mod.id)] = `./${base(mod.name).replace(/ \+ \d+ modules$/, '')}`;
+      acc[String(mod.id)] = normalize(mod.name).replace(/ \+ \d+ modules$/, '');
     return acc;
   }, {});
 }