-
-
Notifications
You must be signed in to change notification settings - Fork 957
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
feat(cli): add sync command for working with external projects #6135
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,97 @@ | |||
import { docsUrl } from '@pnpm/cli-utils' |
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.
@zkochan hello! It's my first time contributing anything other than an error message to pnpm, so if anything looks super wonkey / wrong -- lemme know -- I haven't written tests yet, and the PR is still marked as draft, but I wanted to ping you for early review so that you may have a chance to look at this before I end up spending too much time going down a wrong direction.
This file is < 100 lines 🤷 if that helps prioritization (this is a very busy project!)
for (const packageRelativePath of files) { | ||
const syncFrom = path.join(pkg.dir, packageRelativePath) | ||
const resolvedPackagePath = path.dirname( | ||
resolvePackagePath(pkg.manifest.name, dir) |
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 I'm not sure about -- should I add this package, resolve-package-path
to the package.json? or does pnpm have an existing util for finding the install location for where injected files should go?
This is where sync currently happens: pnpm/exec/lifecycle/src/runLifecycleHooksConcurrently.ts Lines 58 to 84 in e570adc
|
…manage build cache
@zkochan I took a brief look at this again today, and I don't really grok what's going on in the code you linked. I implemented this here: universal-ember/ember-primitives#7 |
📌 This problem can also be solved using https://github.com/tiktok/pnpm-sync |
Reason why: It's hard/impossible to have dependencies automatically injected by default due limitations on hard-links , this PR (for adding
sync
) aims to provide a low-level utility for tools such as turborepo to manage this automatic syncing for us. This is important because when a project starts using peerDependencies, it's unavoidable to have the need fordependenciesMeta.*.injected: true
, yet due to the aforementioned limitations of hard-links, has some major DX downsides -- which this PR and the combination of turborepo (or nx) can 100% alleviate, allowing the average dev to not even know about the problem or the history around these injected issues. Later, it may make sense to have a flag to inject all in-monorepo deps by default, and then rely on this command to fix the hard-link limitations (and this may be required for the feature to be totally transparent to the average dev).Related: #6088
Unblocks: CrowdStrike/ember-headless-table#145