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

Only parse url if it's a string #154

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

rumpl
Copy link

@rumpl rumpl commented Apr 8, 2021

With this change we can use eventsource even when calling a server over a unix domain socket, for example:

const es = new EventSource({
  socketPath: 'some-socket.sock',
  path: '/my-path',
});
...

@rumpl
Copy link
Author

rumpl commented Apr 9, 2021

The failure on node 4.x seems unrelated to my change

 SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:373:25)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/home/runner/work/eventsource/eventsource/node_modules/standard/node_modules/standard-engine/node_modules/deglob/index.js:8:16)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
npm ERR! Test failed.  See above for more details.
Error: Process completed with exit code 1.

@trungutt
Copy link

This simple PR is very helpful to have Unix Domain Socket support. Could anyone have an eye on it please?

@rumpl
Copy link
Author

rumpl commented Nov 24, 2021

The force push is just a rebase with master

@thaJeztah
Copy link

thaJeztah commented May 27, 2022

ping @rexxars PTAL; looks like we're currently maintaining a fork because of this fix enhancement missing in the upstream repository 😓 would be great to see this merged so that we can get rid of the fork.

@thaJeztah
Copy link

@rumpl could you do a rebase to trigger CI?

@rumpl rumpl closed this May 27, 2022
@rumpl rumpl reopened this May 27, 2022
@@ -81,7 +81,10 @@ function EventSource (url, eventSourceInitDict) {
var reconnectUrl = null

function connect () {
var options = parse(url)
var options = url
if (typeof url === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes sense; the spec does say that a URL record is acceptable. Would you mind added a test for this?

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