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

feat(core): add --stdin to affected options #28770

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aaronccasanova
Copy link
Contributor

@aaronccasanova aaronccasanova commented Nov 3, 2024

Current Behavior

Executing nx affected with a large list of files using the --files option frequently results in E2BIG errors during CI runs. This is compounded by our project's deeply nested structure, which returns lengthy file paths even in smaller pull requests.

Expected Behavior

This PR adds a --stdin flag to all affected commands, enabling the list of changed files to be provided via standard input. This approach helps mitigate E2BIG errors by avoiding long command-line arguments. Usage example:

shopify-build changed-files | nx affected -t test --stdin

The use of a --stdin flag for inputing files was inspired by jscodeshift and polaris-migrator. This implementation extends the existing --files option to minimize changes and maintain backward compatibility. Open to alternative suggestions and improvements.

@aaronccasanova aaronccasanova requested review from a team as code owners November 3, 2024 01:44
Copy link

vercel bot commented Nov 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview Jan 14, 2025 4:56pm

Copy link

nx-cloud bot commented Nov 3, 2024

View your CI Pipeline Execution ↗ for commit 5963420.

Command Status Duration Result
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded 37m 26s View ↗
nx run-many -t check-imports check-commit check... ✅ Succeeded 20s View ↗
nx-cloud record -- nx-cloud conformance:check ✅ Succeeded 1s View ↗
nx-cloud record -- nx format:check --base=c6e95... ✅ Succeeded 30s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded 28s View ↗
nx documentation --no-dte ✅ Succeeded 1m 13s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-14 17:34:39 UTC

@aaronccasanova
Copy link
Contributor Author

cc'ing other contributors to these shared options @AgentEnder @FrozenPandaz

@FrozenPandaz
Copy link
Collaborator

Thank you for your PR. I think passing files in via stdin makes sense.

Can we pause to consider if CSV is the best format or if for example, splitting by newlines is better.

The canonical example of why one would use --files is if they used other version management tools such as Mercurial instead of git. For that reason, I think accepting a newline delimited list, as jscodeshift and polaris-migrator do, works better for Mercurial as the command used would be hg status --template "{path}\n". It's a little harder to provide CSV because mercurial would end up with a trailing ,.

It seems like you're using a custom command to list the files. Are there any underlying tools under the hood or was it originally written to be inserted in as nx affected --files "${shopify changed-files}"?

@aaronccasanova
Copy link
Contributor Author

Can we pause to consider if CSV is the best format or if for example, splitting by newlines is better.

I'm impartial and happy to defer to your judgment here. While I omitted the details of our custom command for brevity, we currently transform the results of shopify-build changed-files into a comma-separated list, but adapting it to use newlines is straightforward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants