Skip to content

Conversation

VitaliyR
Copy link
Contributor

🎉 Thanks for submitting a pull request! 🎉

Summary

Adds support for non interactive TTY when running netlify deploy


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@VitaliyR VitaliyR requested a review from a team as a code owner September 23, 2025 20:58
Copy link

github-actions bot commented Sep 23, 2025

📊 Benchmark results

Comparing with 7c1de03

  • Dependency count: 1,081 (no change)
  • Package size: 299 MB (no change)
  • Number of ts-expect-error directives: 380 (no change)
import { parseAllHeaders } from '@netlify/headers-parser'
import { parseAllRedirects } from '@netlify/redirect-parser'
import prettyjson from 'prettyjson'
import { stdin, stdout } from 'process'
Copy link
Member

Choose a reason for hiding this comment

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

nit: Move this to the top, near the other built-in imports.

Comment on lines 123 to 125
// non interactive - can't get the value, resolve to the cwd
deployFolder = command.workspacePackage ? command.jsWorkspaceRoot || site.root : command.workingDir
return deployFolder || command.workingDir
Copy link
Member

Choose a reason for hiding this comment

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

nit: I personally find the nesting of conditionals quite confusing and would prefer a more explicit version, even if more verbose. Up to you, though.

Suggested change
// non interactive - can't get the value, resolve to the cwd
deployFolder = command.workspacePackage ? command.jsWorkspaceRoot || site.root : command.workingDir
return deployFolder || command.workingDir
if (command.workspacePackage) {
if (command.jsWorkspaceRoot) {
return command.jsWorkspaceRoot
}
if (site.root) {
return site.root
}
}
// Either not a workspace package, or empty site root.
return command.workingDir
}

if (!deployFolder) {
if (!stdin.isTTY || !stdout.isTTY) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Might be nice to abstract this to a isInteractive method that we use in the different places we're checking this to ensure consistent behaviour. I could also see us introducing an explicit flag to control this behaviour (similar to npm's --yes) and that would make it easier to integrate. Not a blocker, though.

@VitaliyR VitaliyR merged commit 1e7fc92 into main Sep 24, 2025
67 of 68 checks passed
@VitaliyR VitaliyR deleted the vitaliir/EX-833 branch September 24, 2025 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants