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

[fix] Added missing origin property. #39

Merged
merged 1 commit into from
Oct 31, 2014
Merged

[fix] Added missing origin property. #39

merged 1 commit into from
Oct 31, 2014

Conversation

3rd-Eden
Copy link
Contributor

Fixes #38

Also moved the require(url).parse out of the construction phase. It's a pointless performance hit and the require should be cached.

@@ -184,7 +186,8 @@ function EventSource(url, eventSourceInitDict) {
var type = eventName || 'message';
_emit(type, new MessageEvent(type, {
data: data.slice(0, -1), // remove trailing newline
lastEventId: lastEventId
lastEventId: lastEventId,
origin: original(url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be open to providing an inline definition of original? The lib has 0 dependencies, which I think has some benefits - I'd like to keep it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure we can bundle it with a license reference in the file but that kinda defeats the purpose of having dependencies. Because dependencies can be tested easily individually, maintained individually and shared with the community. If there would be fixes for the original module they will not make their way back to this module. But if you would rather scarifies this for the sake of no modules, i'm fine with that and i'll update the pull request if needed.

@aslakhellesoy aslakhellesoy merged commit c56644e into EventSource:master Oct 31, 2014
@3rd-Eden 3rd-Eden deleted the origin branch October 31, 2014 10:14
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.

Missing origin in message event
2 participants