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.

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

Core: Fix webpack stats #14198

Merged
merged 4 commits into from
Mar 11, 2021
Merged

Core: Fix webpack stats #14198

merged 4 commits into from
Mar 11, 2021

Conversation

tmeasday
Copy link
Member

Issue: #14139

What I did

Forgot to push this final commit on other PR (!)

@tmeasday tmeasday changed the title 14139 fix webpack stats Fix webpack stats Mar 11, 2021
@shilman shilman changed the title Fix webpack stats Core: Fix webpack stats Mar 11, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

CI is failing due to something stats related. can you take a look @tmeasday ?

@tmeasday tmeasday requested a review from shilman March 11, 2021 05:09
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Aha, LGTM 🙏

@shilman shilman merged commit 606050f into next Mar 11, 2021
@shilman shilman deleted the 14139-fix-webpack-stats branch March 11, 2021 10:32
@@ -8,7 +8,7 @@
"do-storyshots-puppeteer": "../../node_modules/.bin/jest --projects=./storyshots-puppeteer",
"generate-addon-jest-testresults": "jest --config=tests/addon-jest.config.json --json --outputFile=stories/addon-jest.testresults.json",
"graphql": "node ./graphql-server/index.js",
"packtracker": "yarn storybook --smoke-test --webpack-stats-json --quiet && cross-env PT_PROJECT_TOKEN=1af1d41b-d737-41d4-ac00-53c8f3913b53 packtracker-upload --stats=./node_modules/.cache/storybook/public/manager-stats.json",
"packtracker": "yarn storybook --smoke-test --webpack-stats-json /tmp --quiet && cross-env PT_PROJECT_TOKEN=1af1d41b-d737-41d4-ac00-53c8f3913b53 packtracker-upload --stats=/tmp/manager-stats.json",
Copy link
Member

@ndelangen ndelangen Mar 11, 2021

Choose a reason for hiding this comment

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

Suggested change
"packtracker": "yarn storybook --smoke-test --webpack-stats-json /tmp --quiet && cross-env PT_PROJECT_TOKEN=1af1d41b-d737-41d4-ac00-53c8f3913b53 packtracker-upload --stats=/tmp/manager-stats.json",
"packtracker": "yarn storybook --smoke-test --webpack-stats-json=/tmp --quiet && cross-env PT_PROJECT_TOKEN=1af1d41b-d737-41d4-ac00-53c8f3913b53 packtracker-upload --stats=/tmp/manager-stats.json",

Comment on lines +42 to +47
export const makeStatsFromError: (err: string) => Stats = (err) =>
({
hasErrors: () => true,
hasWarnings: () => false,
toJson: () => ({ warnings: [] as any[], errors: [err] }),
} as any);
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we want to export?

In the future we'll want to map the builders internal stats object to something common for all builders.
Right now we're leaking webpack details from our 2 builders. This is a known issue, but we should be really careful to not make that situation worse.

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking something like SyntacticEvent in React, where the underlaying raw event is still available, but there's a layer on top to smooth out the browser differences. (except we're smoothing out the builder differences)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think that's the plan for 6.3 (or whenever we add our next builder).

We probably shouldn't be exporting this function however, that was an oversight.

'Write Webpack Stats JSON to disk',
resolvePathInStorybookCache(`public/`)
)
.option('--webpack-stats-json [directory]', 'Write Webpack Stats JSON to disk')
Copy link
Member

Choose a reason for hiding this comment

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

You forgot the <directory>

'Write Webpack Stats JSON to disk',
resolvePathInStorybookCache(`public/`)
)
.option('--webpack-stats-json [directory]', 'Write Webpack Stats JSON to disk')
Copy link
Member

Choose a reason for hiding this comment

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

You forgot <directory>

This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants