-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: add Workspace.find_package_by_path
#10493
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
Conversation
As [here](https://linear.app/vercel/issue/TURBO-4724/expose-package-path-in-turborepository). Depending on the call-pattern that we want to support, it might be worthwhile to introduce a cache for ancestor-paths so that subsequent calls only need to traverse-up to a first-common-ancestor.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
AFAICT that deployment failure is unrelated to this code, though I don't know for sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only concerns are around beefing up the tests.
// every ancestor. | ||
#[napi] | ||
pub async fn find_package_by_path(&self, path: String) -> Result<Package, Error> { | ||
let package_mapper = DefaultPackageChangeMapper::new(&self.graph); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that I should've given you a code pointer on. This is the structure we use to determine which files affect which packages. We can use it raw here to do the same purpose of mapping files to packages.
Happy to chat a bit more about this!
match package_mapper.detect_package(&anchored_path) { | ||
turborepo_repository::change_mapper::PackageMapping::All( | ||
_all_package_change_reason, | ||
) => Err(Error::from_reason("file belongs to many packages")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative here would be to possibly return the root package or return that the file change is considered "global".
const workspace = await Workspace.find(MONOREPO_PATH); | ||
|
||
for (const [filePath, result] of [ | ||
["apps/app/src/util/useful-file.ts", "app-a"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have some tests for root files e.g. a root tsconfig.json
, pnpm-lock.yaml
, pnpm-workspace.yaml
Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>
### Description Bump version to release #10493 ### Testing Instructions 👀
Description
As raised here.
Depending on the call-pattern that we want to support, it might be worthwhile to introduce a cache for ancestor-paths so that subsequent calls only need to traverse-up to a first-common-ancestor.
Testing Instructions
pnpm build && pnpm test
withinpackages/turbo-repository
passes.Note
I'm very much a Rust newbie, so please don't hold back on stylistic nits or comments - I have no strong opinions on "correct" Rust!