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

Send project metadata to the index #1122

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Nov 13, 2024

Reverts #1121 (re-implements #1112) and incorporates #1120.

Also:

  • added some error/timeout handling so we don't get stuck if there is something wrong with git.
  • shorted timeouts to 5s as it's not critical this works (we could shorten further?)
  • don't use wc or head as they may not be there. Instead:
    • simulate head -n 1 via streams
    • just count the number of lines for wc.
      • This is inefficient as it requires loading all the lines into memory.
      • I'm not sure if this will be a problem or not if a repo has a heap of contributors or "app files".
      • I could implement another execGitCommandCountLines but I figured it was better to keep the complexity down.

Things to check in QA:

  • environments without access to head or wc.
  • environments where these commands timeout [not sure how to simulate]
📦 Published PR as canary version: 11.20.0--canary.1122.12075262360.0

✨ Test out this PR locally via:

npm install chromatic@11.20.0--canary.1122.12075262360.0
# or 
yarn add chromatic@11.20.0--canary.1122.12075262360.0

Copy link
Contributor

github-actions bot commented Nov 13, 2024

📦 Package Size: 5448 KB

Copy link

codacy-production bot commented Nov 13, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 691ad521 91.71% (target: 80.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (691ad52) Report Missing Report Missing Report Missing
Head commit (6daedce) 7295 5024 68.87%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#1122) 217 199 91.71%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

@tmeasday tmeasday added enhancement Classification: New feature or request release Auto: Create a `latest` release when merged minor Auto: Increment the minor version when merged labels Nov 13, 2024
@tmeasday tmeasday marked this pull request as ready for review November 13, 2024 04:57
1. Shorter timeouts, but errors will just lead to undefined
2. Don't pipe to `head` or `wc` as they may not exist
@tmeasday tmeasday force-pushed the revert-1121-revert-1112-tom/cap-2327-track-hasrouter-and-haspagecomponents-on-build-events branch from 15cad05 to 3221fcb Compare November 13, 2024 05:00
@jmhobbs
Copy link
Contributor

jmhobbs commented Nov 13, 2024

just count the number of lines for wc.
This is inefficient as it requires loading all the lines into memory.

I think it's a good trade off to add a execGitCommandCountLines. I'd rather we read newlines out of the stream and discard with one additional function than keep unbounded values in memory, twice after the .split('\n')

@tmeasday
Copy link
Member Author

tmeasday commented Nov 14, 2024

@jmhobbs done! Looks like it still works (2 and 0 are the correct values for the repo I was testing with):

image

Copy link
Contributor

@jmhobbs jmhobbs left a comment

Choose a reason for hiding this comment

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

Haven't QA'd, and there's some CI issues, but overall looks right to me.

node-src/lib/getDependentStoryFiles.test.ts Outdated Show resolved Hide resolved
@tmeasday
Copy link
Member Author

@jmhobbs FYI the CI issues (outside of Codacy) is due to the production index not yet having projectMetadata.storybookBaseDir supported yet (need to merge the relevant PR on the other end). This is a good thing I guess (we can't merge this until that's done!)

@tmeasday tmeasday force-pushed the revert-1121-revert-1112-tom/cap-2327-track-hasrouter-and-haspagecomponents-on-build-events branch from 134bbb3 to 6daedce Compare November 28, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Classification: New feature or request minor Auto: Increment the minor version when merged release Auto: Create a `latest` release when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants