Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Read Webpack stats file as stream to support very large projects #624

Merged
merged 4 commits into from Aug 10, 2022
Merged

Read Webpack stats file as stream to support very large projects #624

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44e3097
Update stats process to use stream
ethriel3695 Aug 3, 2022
60eb472
test: mock readStatsFile in upload.test
yannbf Aug 5, 2022
3eb981d
Refactor to isolate read stats functionality
ethriel3695 Aug 8, 2022
de899d6
mock out readStats in upload process
ethriel3695 Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion bin-src/tasks/upload.ts
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ import {
} from '../ui/tasks/upload';
import { Context, Task } from '../types';
import { exitCodes, setExitCode } from '../lib/setExitCode';
import { readStatsFile } from '../trim-stats-file';
ethriel3695 marked this conversation as resolved.
Show resolved Hide resolved

const GetUploadUrlsMutation = `
mutation GetUploadUrlsMutation($paths: [String!]!) {
Expand Down Expand Up @@ -154,7 +155,7 @@ export const traceChangedFiles = async (ctx: Context, task: Task) => {
const statsPath = join(ctx.sourceDir, ctx.fileInfo.statsPath);
const { changedFiles } = ctx.git;
try {
const stats = await fs.readJson(statsPath);
const stats = await readStatsFile(statsPath);
const onlyStoryFiles = await getDependentStoryFiles(ctx, stats, statsPath, changedFiles);
if (onlyStoryFiles) {
ctx.onlyStoryFiles = onlyStoryFiles;
Expand Down
4 changes: 2 additions & 2 deletions bin-src/trace.ts
View file
Open in desktop
@@ -1,7 +1,7 @@
import fs from 'fs-extra';
import meow from 'meow';
import { getDependentStoryFiles } from './lib/getDependentStoryFiles';
import { Context } from './types';
import { readStatsFile } from './trim-stats-file';

/**
* Utility to trace a set of changed file paths to dependent story files using a Webpack stats file.
Expand Down Expand Up @@ -85,7 +85,7 @@ export async function main(argv: string[]) {
traceChanged: flags.mode || true,
},
} as any;
const stats = await fs.readJson(flags.statsFile);
const stats = await readStatsFile(flags.statsFile);
const changedFiles = input.map((f) => f.replace(/^\.\//, ''));

await getDependentStoryFiles(ctx, stats, flags.statsFile, changedFiles);
Expand Down
10 changes: 7 additions & 3 deletions bin-src/trim-stats-file.ts
View file
Open in desktop
@@ -1,5 +1,5 @@
import { parseChunked } from '@discoveryjs/json-ext';
import { createReadStream, outputFile } from 'fs-extra';
import { createReadStream, outputFile, existsSync } from 'fs-extra';
import { Stats } from './types';

const dedupe = <T>(arr: T[]) => Array.from(new Set(arr));
Expand All @@ -8,8 +8,12 @@ const isUserCode = ({ name, moduleName = name }: { name?: string; moduleName?: s
!moduleName.startsWith('(webpack)') &&
!moduleName.match(/(node_modules|webpack\/runtime)\//);

export const readStatsFile = async (filePath: string): Promise<Stats> =>
parseChunked(createReadStream(filePath));
export const readStatsFile = async (filePath: string): Promise<Stats> => {
if (existsSync(filePath)) {
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 added this check because I found that in the upload.test.ts test for traced files, it was erroring out because the file path does not return an existing file.

I attempted to mock the file path but because we mock fs-extra it overrides all my mock tests and cannot find the file.

The logic check ensures that we don't error out on a stats file with a directory that we cannot process.

However, once the trace or upload reaches the point where it's processing the stats file, we have already checked for a file path and that the file exists so we should be able to reasonably assume that the file exists and this is probably an indicator that the test needs to be refactored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is strange is that when you include a combination of the sourceDir and statsPath the upload process builds a path that would most likely not exist in production.

sourceDir: '/static/',
fileInfo: { statsPath: '/static/preview-stats.json' },

This is the path: static/static/preview-stats.json
This is the existing test so I'm hesitant to change it but that file path does not seem like something that would ever exist in a real project.

So is the test incorrect, is the path join incorrect, or is this expected?
@ghengeveld @tmeasday

Copy link
Member

Choose a reason for hiding this comment

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

@ethriel3695 reading where statsPath comes from, it is relative to the sourceDir:

chromatic-cli/bin-src/tasks/upload.ts

Lines 77 to 117 in 0d1bd21

// Get all paths in rootDir, starting at dirname.
// We don't want the paths to include rootDir -- so if rootDir = storybook-static,
// paths will be like iframe.html rather than storybook-static/iframe.html
function getPathsInDir(ctx: Context, rootDir: string, dirname = '.'): PathSpec[] {
try {
return fs.readdirSync(join(rootDir, dirname)).flatMap((p: string) => {
const pathname = join(dirname, p);
const stats = fs.statSync(join(rootDir, pathname));
return stats.isDirectory()
? getPathsInDir(ctx, rootDir, pathname)
: [{ pathname, contentLength: stats.size }];
});
} catch (e) {
ctx.log.debug(e);
throw new Error(invalid({ sourceDir: rootDir } as any, e).output);
}
}
function getOutputDir(buildLog: string) {
const outputString = 'Output directory: ';
const outputIndex = buildLog.lastIndexOf(outputString);
if (outputIndex === -1) return undefined;
const remainingLog = buildLog.slice(outputIndex + outputString.length);
const newlineIndex = remainingLog.indexOf('\n');
const outputDir = newlineIndex === -1 ? remainingLog : remainingLog.slice(0, newlineIndex);
return outputDir.trim();
}
function getFileInfo(ctx: Context, sourceDir: string) {
const lengths = getPathsInDir(ctx, sourceDir).map((o) => ({ ...o, knownAs: slash(o.pathname) }));
const total = lengths.map(({ contentLength }) => contentLength).reduce((a, b) => a + b, 0);
const paths: string[] = [];
let statsPath: string;
// eslint-disable-next-line no-restricted-syntax
for (const { knownAs } of lengths) {
if (knownAs.endsWith('preview-stats.json')) statsPath = knownAs;
else if (!knownAs.endsWith('manager-stats.json')) paths.push(knownAs);
}
return { lengths, paths, statsPath, total };
}

So I think that simply the test is wrong here.

return parseChunked(createReadStream(filePath));
}
return undefined;
};

/**
* Utility to trim down a `preview-stats.json` file to the bare minimum, so that it can be used to
Expand Down