diff --git a/bin/git/git.js b/bin/git/git.js index 5f9385ce5..c53db0064 100644 --- a/bin/git/git.js +++ b/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); } diff --git a/bin/lib/getDependentStoryFiles.js b/bin/lib/getDependentStoryFiles.js index f9a67f405..186d9664f 100644 --- a/bin/lib/getDependentStoryFiles.js +++ b/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,22 +57,22 @@ 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; } }); @@ -66,8 +80,8 @@ export async function getDependentStoryFiles(ctx, stats, changedFiles) { 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 = {}; @@ -75,14 +89,14 @@ export async function getDependentStoryFiles(ctx, stats, changedFiles) { 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; }, {}); } diff --git a/bin/lib/getDependentStoryFiles.test.js b/bin/lib/getDependentStoryFiles.test.js index e74406d6c..f9db2d312 100644 --- a/bin/lib/getDependentStoryFiles.test.js +++ b/bin/lib/getDependentStoryFiles.test.js @@ -1,4 +1,4 @@ -import { getDependentStoryFiles } from './getDependentStoryFiles'; +import { getDependentStoryFiles, normalizePath } from './getDependentStoryFiles'; import { getRepositoryRoot } from '../git/git'; import { getWorkingDir } from './utils'; @@ -11,12 +11,17 @@ const ctx = { log: { warn: jest.fn(), debug: jest.fn() }, }; +beforeEach(() => { + ctx.log.warn.mockReset(); + ctx.log.debug.mockReset(); +}); + getRepositoryRoot.mockResolvedValue('/path/to/project'); getWorkingDir.mockReturnValue(''); describe('getDependentStoryFiles', () => { it('detects direct changes to CSF files', async () => { - const changedFiles = ['./src/foo.stories.js']; + const changedFiles = ['src/foo.stories.js']; const modules = [ { id: 1, @@ -31,12 +36,12 @@ describe('getDependentStoryFiles', () => { ]; const res = await getDependentStoryFiles(ctx, { modules }, changedFiles); expect(res).toEqual({ - 1: './src/foo.stories.js', + 1: 'src/foo.stories.js', }); }); it('detects indirect changes to CSF files', async () => { - const changedFiles = ['./src/foo.js']; + const changedFiles = ['src/foo.js']; const modules = [ { id: 1, @@ -56,13 +61,13 @@ describe('getDependentStoryFiles', () => { ]; const res = await getDependentStoryFiles(ctx, { modules }, changedFiles); expect(res).toEqual({ - 2: './src/foo.stories.js', + 2: 'src/foo.stories.js', }); }); - it('supports webpack root in git subdirectory', async () => { + it('supports webpack projectRoot in git subdirectory', async () => { getWorkingDir.mockReturnValueOnce('services/webapp'); - const changedFiles = ['./services/webapp/src/foo.js']; + const changedFiles = ['services/webapp/src/foo.js']; const modules = [ { id: 1, @@ -82,14 +87,14 @@ describe('getDependentStoryFiles', () => { ]; const res = await getDependentStoryFiles(ctx, { modules }, changedFiles); expect(res).toEqual({ - 2: './src/foo.stories.js', + 2: 'services/webapp/src/foo.stories.js', }); }); it('supports absolute module paths', async () => { getRepositoryRoot.mockResolvedValueOnce('/path/to/project'); const absoluteCsfGlob = `/path/to/project/${CSF_GLOB.slice(2)}`; - const changedFiles = ['./src/foo.js']; + const changedFiles = ['src/foo.js']; const modules = [ { id: 1, @@ -110,24 +115,25 @@ describe('getDependentStoryFiles', () => { ]; const res = await getDependentStoryFiles(ctx, { modules }, changedFiles); expect(res).toEqual({ - 2: './src/foo.stories.js', + 2: 'src/foo.stories.js', }); }); it('supports absolute module paths with deviating working dir', async () => { getRepositoryRoot.mockResolvedValueOnce('/path/to/project'); - getWorkingDir.mockReturnValueOnce('services/webapp'); - const absoluteCsfGlob = `/path/to/project/services/webapp/${CSF_GLOB.slice(2)}`; - const changedFiles = ['./services/webapp/src/foo.js']; + getWorkingDir.mockReturnValueOnce('packages/storybook'); // note this is a different workspace + + const absoluteCsfGlob = `/path/to/project/packages/webapp/${CSF_GLOB.slice(2)}`; + const changedFiles = ['packages/webapp/src/foo.js']; const modules = [ { id: 1, - name: '/path/to/project/services/webapp/src/foo.js', - reasons: [{ moduleName: '/path/to/project/services/webapp/src/foo.stories.js' }], + name: '/path/to/project/packages/webapp/src/foo.js', + reasons: [{ moduleName: '/path/to/project/packages/webapp/src/foo.stories.js' }], }, { id: 2, - name: '/path/to/project/services/webapp/src/foo.stories.js', + name: '/path/to/project/packages/webapp/src/foo.stories.js', reasons: [{ moduleName: absoluteCsfGlob }], }, { @@ -139,7 +145,136 @@ describe('getDependentStoryFiles', () => { ]; const res = await getDependentStoryFiles(ctx, { modules }, changedFiles); expect(res).toEqual({ - 2: './src/foo.stories.js', + 2: 'packages/webapp/src/foo.stories.js', + }); + }); + + it('bails on changed global file', async () => { + const changedFiles = ['src/foo.stories.js', 'src/package.json']; + const modules = [ + { + id: 1, + name: './src/foo.stories.js', + reasons: [{ moduleName: CSF_GLOB }], + }, + { + id: 999, + name: CSF_GLOB, + reasons: [{ moduleName: './.storybook/generated-stories-entry.js' }], + }, + ]; + const res = await getDependentStoryFiles(ctx, { modules }, changedFiles); + expect(res).toEqual(false); + expect(ctx.log.warn).toHaveBeenCalledWith( + expect.stringContaining('Found a change in src/package.json') + ); + }); + + it('bails on changed config file', async () => { + const changedFiles = ['src/foo.stories.js', 'path/to/storybook-config/file.js']; + const modules = [ + { + id: 1, + name: './src/foo.stories.js', + reasons: [{ moduleName: CSF_GLOB }], + }, + { + id: 999, + name: CSF_GLOB, + reasons: [{ moduleName: './.storybook/generated-stories-entry.js' }], + }, + ]; + const res = await getDependentStoryFiles( + { ...ctx, storybook: { configDir: 'path/to/storybook-config' } }, + { modules }, + changedFiles + ); + expect(res).toEqual(false); + expect(ctx.log.warn).toHaveBeenCalledWith( + expect.stringContaining('Found a change in path/to/storybook-config/file.js') + ); + }); + + it('bails on changed static file', async () => { + const changedFiles = ['src/foo.stories.js', 'path/to/statics/image.png']; + const modules = [ + { + id: 1, + name: './src/foo.stories.js', + reasons: [{ moduleName: CSF_GLOB }], + }, + { + id: 999, + name: CSF_GLOB, + reasons: [{ moduleName: './.storybook/generated-stories-entry.js' }], + }, + ]; + const res = await getDependentStoryFiles( + { ...ctx, storybook: { staticDir: ['path/to/statics'] } }, + { modules }, + changedFiles + ); + expect(res).toEqual(false); + expect(ctx.log.warn).toHaveBeenCalledWith( + expect.stringContaining('Found a change in path/to/statics/image.png') + ); + }); +}); + +describe('normalizePath', () => { + it('converts absolute paths to relative paths', () => { + const projectRoot = '/path/to/project'; + const normalized = normalizePath(`${projectRoot}/src/webapp/file.js`, projectRoot); + expect(normalized).toBe('src/webapp/file.js'); + }); + + it('returns already relative paths as-is', () => { + const projectRoot = '/path/to/project'; + const normalized = normalizePath(`src/webapp/file.js`, projectRoot); + expect(normalized).toBe('src/webapp/file.js'); + }); + + it('handles complex relative paths', () => { + const projectRoot = '/path/to/project'; + const normalized = normalizePath(`../webapp/file.js`, projectRoot); + expect(normalized).toBe('../webapp/file.js'); + }); + + describe('with workingDir', () => { + it('makes relative paths relative to the project root', () => { + const projectRoot = '/path/to/project'; + const normalized = normalizePath(`folder/file.js`, projectRoot, 'packages/webapp'); + expect(normalized).toBe('packages/webapp/folder/file.js'); + }); + + it('handles complex relative paths', () => { + const projectRoot = '/path/to/project'; + const normalized = normalizePath( + `../../packages/webapp/folder/file.js`, + projectRoot, + 'packages/tools' + ); + expect(normalized).toBe('packages/webapp/folder/file.js'); + }); + + it('converts absolute paths to relative paths as normal', () => { + const projectRoot = '/path/to/project'; + const normalized = normalizePath( + `${projectRoot}/packages/webapp/file.js`, + projectRoot, + 'packages/webapp' + ); + expect(normalized).toBe('packages/webapp/file.js'); + }); + + it('does not affect paths outside of working dir', () => { + const projectRoot = '/path/to/project'; + const normalized = normalizePath( + `${projectRoot}/packages/webapp/file.js`, + projectRoot, + 'packages/tools' + ); + expect(normalized).toBe('packages/webapp/file.js'); }); }); }); diff --git a/bin/lib/getOptions.js b/bin/lib/getOptions.js index b08cc6e82..65fb73673 100644 --- a/bin/lib/getOptions.js +++ b/bin/lib/getOptions.js @@ -5,6 +5,7 @@ import duplicatePatchBuild from '../ui/messages/errors/duplicatePatchBuild'; import incompatibleOptions from '../ui/messages/errors/incompatibleOptions'; import invalidExitOnceUploaded from '../ui/messages/errors/invalidExitOnceUploaded'; import invalidOnly from '../ui/messages/errors/invalidOnly'; +import invalidOnlyChanged from '../ui/messages/errors/invalidOnlyChanged'; import invalidPatchBuild from '../ui/messages/errors/invalidPatchBuild'; import invalidReportPath from '../ui/messages/errors/invalidReportPath'; import invalidSingularOptions from '../ui/messages/errors/invalidSingularOptions'; @@ -147,6 +148,10 @@ export default async function getOptions({ argv, env, flags, log, packageJson }) // Build Storybook instead of starting it if (!scriptName && !exec && !noStart && !storybookUrl && !port) { if (storybookBuildDir) { + if (options.onlyChanged) { + // TurboSnap requires that we build the Storybook, otherwise absolute paths won't match up. + throw new Error(incompatibleOptions(['--storybook-build-dir (-d)', '--only-changed'])); + } return { ...options, noStart: true, useTunnel: false }; } const { scripts } = packageJson; @@ -163,6 +168,9 @@ export default async function getOptions({ argv, env, flags, log, packageJson }) throw new Error(missingBuildScriptName(buildScriptName)); } + // TurboSnap requires generating a static build with a webpack stats file. + if (options.onlyChanged) throw new Error(invalidOnlyChanged()); + // Start Storybook on localhost and generate the URL to it if (!storybookUrl) { if (exec && !port) { diff --git a/bin/lib/utils.js b/bin/lib/utils.js index 5fecc96e4..890610257 100644 --- a/bin/lib/utils.js +++ b/bin/lib/utils.js @@ -33,4 +33,4 @@ export const rewriteErrorMessage = (err, message) => { } }; -export const getWorkingDir = (basePath) => path.relative(basePath, '.'); +export const getWorkingDir = (basePath) => path.posix.relative(basePath, '.'); diff --git a/bin/tasks/gitInfo.js b/bin/tasks/gitInfo.js index fbd05178d..e0fb58be7 100644 --- a/bin/tasks/gitInfo.js +++ b/bin/tasks/gitInfo.js @@ -111,7 +111,7 @@ export const setGitInfo = async (ctx, task) => { try { const results = await Promise.all(baselineCommits.map((c) => getChangedFiles(c))); - ctx.git.changedFiles = [...new Set(results.flat())].map((f) => `./${f}`); + ctx.git.changedFiles = [...new Set(results.flat())]; ctx.log.debug(`Found changedFiles:\n${ctx.git.changedFiles.map((f) => ` ${f}`).join('\n')}`); } catch (e) { ctx.git.changedFiles = null; diff --git a/bin/tasks/gitInfo.test.js b/bin/tasks/gitInfo.test.js index 6db363fe4..7e1d8a8e6 100644 --- a/bin/tasks/gitInfo.test.js +++ b/bin/tasks/gitInfo.test.js @@ -46,7 +46,7 @@ describe('setGitInfo', () => { getChangedFiles.mockResolvedValue(['styles/main.scss', 'lib/utils.js']); const ctx = { log, options: { onlyChanged: true } }; await setGitInfo(ctx, {}); - expect(ctx.git.changedFiles).toEqual(['./styles/main.scss', './lib/utils.js']); + expect(ctx.git.changedFiles).toEqual(['styles/main.scss', 'lib/utils.js']); }); it('drops changedFiles when matching --externals', async () => { diff --git a/bin/ui/messages/errors/invalidOnlyChanged.js b/bin/ui/messages/errors/invalidOnlyChanged.js new file mode 100644 index 000000000..ecea91283 --- /dev/null +++ b/bin/ui/messages/errors/invalidOnlyChanged.js @@ -0,0 +1,10 @@ +import chalk from 'chalk'; +import dedent from 'ts-dedent'; + +import { error } from '../../components/icons'; + +export default () => + dedent(chalk` + ${error} Invalid {bold --only-changed} + This option is only supported when you use an uploaded build. + `); diff --git a/bin/ui/messages/errors/invalidOnlyChanged.stories.js b/bin/ui/messages/errors/invalidOnlyChanged.stories.js new file mode 100644 index 000000000..420bba1c0 --- /dev/null +++ b/bin/ui/messages/errors/invalidOnlyChanged.stories.js @@ -0,0 +1,7 @@ +import invalidOnlyChanged from './invalidOnlyChanged'; + +export default { + title: 'CLI/Messages/Errors', +}; + +export const InvalidOnlyChanged = () => invalidOnlyChanged(); diff --git a/package.json b/package.json index 7ebeed1bb..ecc53c062 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "chromatic", - "version": "5.10.0-canary.3", + "version": "5.10.0-canary.4", "description": "Automate visual testing across browsers. Gather UI feedback. Versioned documentation.", "keywords": [ "storybook-addon",