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

Scripts: Stop using shebangs to prevent issues with Windows environments #32504

Closed
gziolo opened this issue Jun 8, 2021 · 2 comments
Closed
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality

Comments

@gziolo
Copy link
Member

gziolo commented Jun 8, 2021

What problem does this address?

Related issue: #32258.

Related comment in #31279 (comment) by @mcsf:

As much as I love shebangs, AFAIK they just don't work well or at all with Windows. This just emerged in #32329. I'm wondering if we should/could forbid it with our linter — wdyt?

On Windows when running a script that uses shebangs you will encounter the following error message:

'.' is not recognized as an internal or external command,
operable program or batch file.

What is your proposed solution?

Related comment by @mcsf in #31279 (comment):

The alternative would be to just not have any shebang, meaning that scripts would just have code:

1. /**
2.  * External dependencies
3.  */
4. const builder = ...;

This would make it impossible for anyone in a UNIX system to invoke ./index.js (they would have to explicitly call the interpreter with node index.js), thus eliminating the possibility of merging something that would break in Windows.

The task is to find all scripts that use shebang like:

#!/usr/bin/env node

and check if they work correctly with Windows. Some might work like:

#!/usr/bin/env node

because they are linked in the bin folder in node_modules:

"bin": {
"wp-create-block": "./index.js"
},

@mcsf, how do we decide? 😄

@gziolo gziolo added [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts labels Jun 8, 2021
@mcsf
Copy link
Contributor

mcsf commented Jun 8, 2021

Hm, I wasn't aware of the special treatment of scripts linked via the bin property in package.json. The docs even stress:

Please make sure that your file(s) referenced in bin starts with #!/usr/bin/env node, otherwise the scripts are started without the node executable!

Which leads me to think that Node must do some special dance in Windows runtimes in order to handle these well.

In that case, I withdraw my recommendation to remove shebangs. I think trying to discern good shebangs from bad ones will be difficult, and with little payoff. If any other reports from Windows users come our way, we'll patch those issues ad hoc.

If you agree, you can close this issue. In that case, I'm sorry to have wasted your time with this, @gziolo. :)

@gziolo gziolo closed this as completed Jun 8, 2021
@gziolo
Copy link
Member Author

gziolo commented Jun 8, 2021

Interesting learning about the recommendation for package.json setup related to scripts linked via bin 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

2 participants