Repository navigation
fix(@schematics/update): Allow usage strict SSL configuration - #838
Conversation
| return concat( | ||
| getNpmConfigOption('proxy'), | ||
| getNpmConfigOption('https-proxy'), | ||
| getNpmConfigOption('strict-ssl'), |
There was a problem hiding this comment.
Is getNpmConfigOption('strict-ssl') provides true default when strict-ssl is missing in config?
| https: options[1], | ||
| }, | ||
| ssl: { | ||
| strict: options[2], |
There was a problem hiding this comment.
This will give you the string value instead of the boolean. options[2] === 'true' would be more appropriate.
There was a problem hiding this comment.
Since npm-registry-client's config requires ssl.strict to be a boolean and npm's default config has strict-ssl set to true (https://docs.npmjs.com/misc/config#strict-ssl), the exec npm get strict-ssl in getNpmConfigOption function should yield true if not explicitly set.
options[2] === 'true' should suffice as @alvaro450 mentioned above.
| https: options[1], | ||
| }, | ||
| ssl: { | ||
| strict: options[2], |
There was a problem hiding this comment.
Since npm-registry-client's config requires ssl.strict to be a boolean and npm's default config has strict-ssl set to true (https://docs.npmjs.com/misc/config#strict-ssl), the exec npm get strict-ssl in getNpmConfigOption function should yield true if not explicitly set.
options[2] === 'true' should suffice as @alvaro450 mentioned above.
| https: options[1], | ||
| }, | ||
| ssl: { | ||
| strict: options[2] === 'true', |
There was a problem hiding this comment.
If it is undefined it should stay as undefined to pick up the default.
There was a problem hiding this comment.
undefined will cause strict: false. strict is default to true. The key should go there only if strict is false
if(options[2]==='false') {
opts = {...opts, {ssl: {strict: false}}}
}
ssl stuff should be maximally explicit
There was a problem hiding this comment.
Actually, undefined will cause it to be true. It is explicitly handled: https://github.com/npm/npm-registry-client/blob/latest/index.js#L32
However, there is not much downside to being explicit here as well. I'd probably go with something similar to the following:
...(options[2] === 'false' ? { strict: false } : (options[2] === 'true' ? { strict: true } : {})),
There was a problem hiding this comment.
Addressed and updated
8 participants
Fixes angular/angular-cli#10596