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

Core: Improve project root detection logic #20791

Merged
merged 10 commits into from
Nov 27, 2023
Merged

Core: Improve project root detection logic #20791

merged 10 commits into from
Nov 27, 2023

Conversation

dobesv
Copy link
Contributor

@dobesv dobesv commented Jan 26, 2023

  • Support .hg
  • Support .yarn
  • Support an env var STORYBOOK_PROJECT_ROOT as an override if other detection methods are not working

Issues: #20314 #14042 #20211

What I did

Update getProjectRoot logic to cover additional use cases.

How to test

See the related issues for the cases that lead to this.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

@dobesv
Improve project root detection logic
a61a911 
- Support `.hg`
- Support `.yarn`
- Support an env var STORYBOOK_PROJECT_ROOT as an override if other detection methods are not working
@kylegach kylegach added the ci:daily Run the CI jobs that normally run in the daily job. label Jan 30, 2023
@kylegach
Copy link
Contributor

Hey, @dobesv! Thank you for this contribution (and congratulations on your first!). Could you add some tests for this, please?

@dobesv
Copy link
Contributor Author

dobesv commented Jan 30, 2023

Any pointers on how and where to add the tests? I'm not too familiar with the project, and I don't think there are existing tests for this function unfortunately. It seems a bit tricky to test.

ndelangen
ndelangen requested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

I left 2 comments

@ndelangen
Copy link
Member

@dobesv thank you for contributiong! I think this change is good, but I'd like it implemented in a slightly different way, I hope you can make that happen? 🙏

@dobesv
Fixes
7ed1551 
- Stop checking for VCS folders if we find one
- Use __dirname for the .yarn/node_modules search
- Also consider `.yarn` along with VCS folders to indicate the project root
@dobesv
Copy link
Contributor Author

dobesv commented Jan 31, 2023

Thanks for the review @ndelangen . I made changes based on your feedback. If you have any thoughts about how I could make automated tests for this without a ton of work, let me know.

@ndelangen
Copy link
Member

ndelangen commented Feb 1, 2023 •
edited
Loading

We could add tests that mock find-up; that would mean we only have to specify when find-up with returns with what string..

@ndelangen ndelangen changed the title Improve project root detection logic Core: Improve project root detection logic Nov 24, 2023
@ndelangen ndelangen added ci:daily Run the CI jobs that normally run in the daily job. ci:normal and removed ci:daily Run the CI jobs that normally run in the daily job. labels Nov 24, 2023
@ndelangen ndelangen changed the base branch from next to release-8-0 November 24, 2023 15:14
@ndelangen ndelangen merged commit 20600b3 into storybookjs:release-8-0 Nov 27, 2023
48 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndelangen ndelangen ndelangen approved these changes

@yannbf yannbf Awaiting requested review from yannbf

Assignees

@ndelangen ndelangen

Labels
ci:normal feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dobesv @kylegach @ndelangen