-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(core): add globPatternforDependencies support nx:run-commands wi… #12786
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
const initialCwd = process.cwd(); | ||
if (initialCwd !== workspaceRoot) process.chdir(workspaceRoot); |
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.
Rather than changing process.cwd
, we should be able to resolve the paths relative to workspaceRoot. The changeset would probably be a bit larger, but I think it would be preferred.
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.
I agree. I initially went on that track but stumbled across issues that seemed to go deeper than the scope of globPatternforDependencies
, possibly in the ignore
library. But i'll revisit!
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.
IIRC ignore
requires that the paths are relative. Might need to resolve, and then make relative rather than just going w/ the fully resolved paths.
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.
Changed implementation as suggested.
I was able to solve my original issue with the ignore
lib which stemmed from the 5.0.0 update to the library.
See: kaelzhang/node-ignore#20 for context.
I was trying to pass relative paths as ../foo/bar
which it does not accept and the thrown error was a bit misguiding:
path should be a path.relative()d string, but got "../foo/bar"
and in my world ../foo/bar
is a "relative()d" string :)
…th cwd option As globPatternforDependencies expects cwd to be in workspaceRoot, it will fail if it's used in a script that is called via nx:run-commands with `cwd` option pointing to project. This \"fix\" changes the process cwd to workspaceRoot while running the globPatternforDependencies script, then changes back. ISSUES CLOSED: #12721
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
…th cwd option
As globPatternforDependencies expects cwd to be in workspaceRoot, it will fail if it's used in a script that is called via
nx:run-commands
withcwd
option pointing to project. This "fix" resolves paths to allow the script to run even from project dir.Closes issues: #12721
Current Behavior
When running a script that utilises
globPatternforDependencies
from workspace package in a target that usesnx:run-commands
executor together withcwd
option pointing to project root then that script will fail because it can not resolve the paths correctly.An example would be a migrated CRA application that should run
craco start
from project root and contains a tailwind config per documentation.Expected Behavior
Running target with
nx:run-commands
withcwd
option should not breakglobPatternforDependencies
.Related Issue(s)
Fixes #12721
EDIT(26/10/2022): Edited due to changed fix implementation