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

Add proxy option #319

Closed
wants to merge 8 commits into from
Closed

Add proxy option #319

wants to merge 8 commits into from

Conversation

andolini
Copy link

@andolini andolini commented Mar 16, 2018

Fixes #280.



if('proxy' in options && typeof options.proxy === 'string') {
httpsOptions.agent = new HttpsProxyAgent(options.proxy);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you separate the check for the type of options.proxy and print a console warning if it's set but not used because it's not a string.

I'd also be happy with throwing an error.

@gauntface
Copy link

Could you also add a test (along the lines of

test('Extra headers', function() {
)

@@ -233,7 +233,8 @@ suite('Test Generate Request Details', function() {
headers: {
'Topic': 'topic',
'Urgency': 'urgency'
}
},
'proxy': 'proxy'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A separate test should be added for the proxy option, this test is for the Extra headers parameter.

@marco-c
Copy link
Member

marco-c commented Mar 28, 2018

It would be nice to have a test in test/testSendNotification.js too, but we can do it in a follow-up.

@marco-c
Copy link
Member

marco-c commented Mar 28, 2018

The Travis build with the stable version of Node is failing with:

Error: Cannot find module 'ms'
    at Function.Module._resolveFilename (module.js:543:15)
    at Function.Module._load (module.js:470:25)
    at Module.require (module.js:593:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/home/travis/build/web-push-libs/web-push/node_modules/debug/src/debug.js:14:20)
    at Module._compile (module.js:649:30)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)

@marco-c marco-c changed the title add proxy option Add proxy option Mar 28, 2018
@@ -4,10 +4,25 @@
"lockfileVersion": 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has a lot of changes that might be unnecessary.

marco-c added a commit that referenced this pull request Mar 29, 2018
@marco-c
Copy link
Member

marco-c commented Mar 29, 2018

Merged manually in f099ec8. Thanks!

@marco-c marco-c closed this Mar 29, 2018
@propr
Copy link

propr bot commented Mar 29, 2018

Please provide your feedback on this pull request here.

Privacy statement: We don't store any personal information such as your email address or name. We ask for GitHub authentication as an anonymous identifier to account for duplicate feedback entries and to see people specific preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants