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 createConnection option for http or https requests #120

Merged
merged 1 commit into from Jan 25, 2020

Conversation

xv2
Copy link
Contributor

@xv2 xv2 commented Mar 13, 2019

issue #119

@thedillonb
Copy link

@thedillonb thedillonb commented Nov 18, 2019

@aslakhellesoy any chance we can get this merged?

@rexxars rexxars merged commit 4ad734b into EventSource:master Jan 25, 2020
1 check passed
@rexxars
Copy link
Member

@rexxars rexxars commented Jan 25, 2020

Thanks for contributing!

@aslakhellesoy
Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Jan 25, 2020

The supplied test won’t fail if createConnection isn’t called. The test just sets a boolean flag which is never checked.

The feature is undocumented. This didn’t seem quite ready to be merged. Sorry for my late review!

@rexxars
Copy link
Member

@rexxars rexxars commented Jan 27, 2020

It's checked on line 611, in the request handler - assert.ok(testResult). done() is not called apart from within that handler. Am I not reading the test correctly?

I'll add documentation for the feature unless you want this reverted.

@aslakhellesoy
Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Jan 27, 2020

You're right @rexxars - that will teach me to stop doing code reviews on my phone :-)

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.

None yet

4 participants