Skip to content
New issue

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

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

Already on GitHub? # to your account

Allow tracing changed files to affected story files CH-1205 CH-1393 #495

Merged
merged 24 commits into from
Jan 17, 2022

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Jan 10, 2022

Depends on https://github.com/chromaui/chromatic/pull/5786

This adds a trace utility and the --trace-changed flag which print a tree visualization of how changed files relate to (bundles containing) affected story files:

Screenshot 2022-01-10 at 11 15 40

Adding the --expanded flag to trace or setting --trace-changed=expanded reveals underlying modules:

Screenshot 2021-12-30 at 17 26 08

npx chromatic trace [...changed file paths] operates on a provided stats file (--stats-file, default storybook-static/preview-stats.json) and provided list of changed files. Alternatively, you can pass the --trace-changed flag to the regular CLI to print this info as part of the Chromatic build (possibly combined with --dry-run).

@linear
Copy link

linear bot commented Jan 10, 2022

@ghengeveld ghengeveld changed the title Add util to trace changed files to affected story files CH-1205 Allow tracing changed files to affected story files CH-1205 Jan 10, 2022
@ghengeveld ghengeveld requested a review from tmeasday January 10, 2022 15:18
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

This is great, a couple questions:

  1. I think maybe you could add some tests to the getDependentStoryFiles just checking the tracedPaths end up looking as we expect? Perhaps the existing tests would work well here?
  2. In terms of testing the UI of the traced paths, it might be nice to have at least one test that will ensure we notice if it breaks (I'm not super worried about the specifics of the UI, but we could do stories?)
  3. I'm confused about trace vs stats-to-story-files -- are they doing the same thing?

@ghengeveld
Copy link
Member Author

  1. I think maybe you could add some tests to the getDependentStoryFiles just checking the tracedPaths end up looking as we expect? Perhaps the existing tests would work well here?

Makes sense. I'll extend one/some of the existing tests with traceChanged and verify the output. Perhaps a Jest snapshot test makes sense for this 🤔

  1. In terms of testing the UI of the traced paths, it might be nice to have at least one test that will ensure we notice if it breaks (I'm not super worried about the specifics of the UI, but we could do stories?)

I think the snapshot test for 👆 would cover breakage. We can do a story for the visual side of it but it would require some refactoring to extract the (visual) text from the code into messages (components). At first I didn't think it was necessary because it was just a util, but now that it's also a flag, we should give it the proper UI treatment.

  1. I'm confused about trace vs stats-to-story-files -- are they doing the same thing?

They both call getDependentStoryFiles and take roughly the same arguments, but their output is different. Basically stats-to-story-files is a compact version of trace. Do you think we should drop stats-to-story-files in favor of trace? Perhaps we can add a compact mode (similar to expanded) which essentially gives the output of stats-to-story-files. In an earlier version, stats-to-story-files actually returned JSON. Do you think that's worthwhile to have?

@tmeasday
Copy link
Member

Perhaps we can add a compact mode (similar to expanded) which essentially gives the output of stats-to-story-files.

I agree that seems like a good idea

Do you think that's worthwhile to have?

JSON output? AFAIK that's not used anywhere so I guess not.

@ghengeveld ghengeveld changed the title Allow tracing changed files to affected story files CH-1205 Allow tracing changed files to affected story files CH-1205 CH-1393 Jan 13, 2022
@linear
Copy link

linear bot commented Jan 13, 2022

CH-1393 Add a storybookBaseDir option to the trace/stats-to-story-files script

Currently you can set the config dir via STORYBOOK_CONFIG_DIR in the environment. Ideally you could do the same with the base dir.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM, 👍 @ghengeveld

@ghengeveld ghengeveld merged commit 01ee7aa into main Jan 17, 2022
@ghengeveld ghengeveld deleted the trace-stats branch January 17, 2022 21:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants