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

[rush] Rush should support the ${NAME:-fallback} syntax in .npmrc #5029

Closed
fzxen opened this issue Dec 4, 2024 · 1 comment · Fixed by #5030
Closed

[rush] Rush should support the ${NAME:-fallback} syntax in .npmrc #5029

fzxen opened this issue Dec 4, 2024 · 1 comment · Fixed by #5030

Comments

@fzxen
Copy link
Contributor

fzxen commented Dec 4, 2024

Summary

I want to set the .npmrc based on environment variables, so I added the following content to the .npmrc:

store-dir=${PNPM_STORE_DIR:-~/.pnpm/store}

This configuration means that when the environment variable PNPM_SOTRE_DIR exists, the store-dir takes its value. If it does not exist, then it takes ~/.pnpm/store

Pnpm supports this feature

Values in the .npmrc files may contain env variables using the ${NAME} syntax. The env variables may also be specified with default values. Using ${NAME-fallback} will return fallback if NAME isn't set. ${NAME:-fallback} will return fallback if NAME isn't set, or is an empty string. —— https://pnpm.io/npmrc

However, After executing "rush install", I found that rush modified the content of the npmrc file. The line "store-dir" was added with the prefix ; MISSING ENVIRONMENT VARIABLE. This causes this configuration to be invalid.

; MISSING ENVIRONMENT VARIABLE: store-dir=${PNPM_STORE_DIR:-~/.pnpm/store}

Repro steps

Add store-dir=${PNPM_STORE_DIR:-~/.pnpm/store} to the common/config/rush/.npmrc file.
Then, execute rush install command.

Expected result:

The content of common/temp/.npmrc displays store-dir=${PNPM_STORE_DIR:-~/.pnpm/store}

Actual result:

The content of common/temp/.npmrc displays ; MISSING ENVIRONMENT VARIABLE: store-dir=${PNPM_STORE_DIR:-~/.pnpm/store}

Details

I located the logical code causing the problem. This code seems to have not considered the fallback syntax.
Actually, I don't understand why the prefix MISSING ENVIRONMENT VARIABLE is added here.

for (let line of npmrcFileLines) {
let lineShouldBeTrimmed: boolean = false;
//remove spaces before or after key and value
line = line
.split('=')
.map((lineToTrim) => lineToTrim.trim())
.join('=');
// Ignore comment lines
if (!commentRegExp.test(line)) {
const environmentVariables: string[] | null = line.match(expansionRegExp);
if (environmentVariables) {
for (const token of environmentVariables) {
// Remove the leading "${" and the trailing "}" from the token
const environmentVariableName: string = token.substring(2, token.length - 1);
// Is the environment variable defined?
if (!process.env[environmentVariableName]) {
// No, so trim this line
lineShouldBeTrimmed = true;
break;
}
}
}
}
if (lineShouldBeTrimmed) {
// Example output:
// "; MISSING ENVIRONMENT VARIABLE: //my-registry.com/npm/:_authToken=${MY_AUTH_TOKEN}"
resultLines.push('; MISSING ENVIRONMENT VARIABLE: ' + line);
} else {
resultLines.push(line);
}
}

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question Answer
@microsoft/rush globally installed version? 5.142.0
rushVersion from rush.json? 5.142.0
Operating system? Mac
Would you consider contributing a PR? Yes
Node.js version (node -v)? 20.16.0
@github-project-automation github-project-automation bot moved this to Needs triage in Bug Triage Dec 4, 2024
@fzxen fzxen changed the title [rush] Rush should support the ${NAME:fallback} syntax in .npmrc [rush] Rush should support the ${NAME:-fallback} syntax in .npmrc Dec 4, 2024
@iclanton
Copy link
Member

iclanton commented Dec 4, 2024

I suspect the fallback feature didn't exist when this was first implemented.

There is(/used to be) a behavior where a missing env variable that a .npmrc depends on would cause an error, so Rush automatically just filters those lines out. Supporting the fallback syntax with a regexp would be pretty easy.

@iclanton iclanton moved this from Needs triage to Low priority in Bug Triage Dec 4, 2024
@github-project-automation github-project-automation bot moved this from Low priority to Closed in Bug Triage Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants