npm / npm/cli

Enhancement: Check for shell builtin before wrapping

Open
#8,542 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
10.1k
Forks
4.7k
Avg merge
2d 2h
Merged PRs (30d)
19

Description

The introduction of wrapping commands given the possibility it is a shell builtin keyword on *nix envs results in potential issues in projects relying on npm_lifecycle_script env var given the value has now changed.
This was introduced in https://github.com/npm/cli/commit/fdc3413019c2f34f1fde35449e5f3a6b0fb51ba2

Instead of wrapping all cmds it would be beneficial to first check if the command is a builtin prior to wrapping.
Something such as ...

  if (!isWindowsShell) {
    if (args.length > 0) {
        let builtin = false;
        exec(`type ${args[0]}`, (error, stdout, stderr) => { if(stdout.includes('keyword')) { builtin = true; } });
        if(builtin) {
          args[0] = '"' + args[0] + '"';
        } 
    }
  }

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing the command-wrapping behavior introduced in commit fdc3413019c2f34f1fde35449e5f3a6b0fb51ba2, then inspect how npm_lifecycle_script changes for shell builtins on Unix environments. Done means builtin commands are checked before wrapping and projects relying on the original environment variable value no longer regress.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, nodejs
Domain
cli
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.