fix: handle npm prefix config environment variables #7070
Conversation
| @@ -171,7 +171,9 @@ export default class BaseRegistry { | |||
| key = key.replace(/__/g, '.'); | |||
|
|
|||
| // replace underscores with dashes | |||
| key = key.replace(/_/g, '-'); | |||
There was a problem hiding this comment.
Can you instead add it in a more generic way that supports __?
For example: key.replace(/([^_])_/g, '$1-')
There was a problem hiding this comment.
That's a good idea. We're probably assuming any key starting with an underscore needs to stay an underscore, but that's probably always true
There was a problem hiding this comment.
@arcanis Are the changes satisfactory?
# Conflicts: # CHANGELOG.md
| test('correctly escapes environment config variables', () => { | ||
| const testCwd = '.'; | ||
| const {mockRequestManager, mockRegistries, mockReporter} = createMocks(); | ||
| const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter, true, []); |
There was a problem hiding this comment.
I had trouble figuring out where these tests should go. mergeEnv is an instance method that requires a whole mocked registry to test.
Simplifying this test would involve refactoring the mergeEnv method to use a utility method, but I decided to reduce the changes to the code and instead have a more complicated test
|
Yep! Thanks 😃 |
Fixes #4682
Summary
Passing
npm_config_environment variables changed all underscores (_) to dashes (-) which isn't correct behavior for special config variables (_auth,_authToken,_usernameand_password). An additional check was added tobase-registry.jsto filter out these config keys from this processing.Test plan
yarn test- all tests passed except two tests that have nothing with my change - they are integration tests (I assume my environment isn't set up correctly for these to pass? Maybe CI will verify?)