socketio / engine.io-client

The engine used in the Socket.IO JavaScript client, which manages the low-level transports such as HTTP long-polling, WebSocket and WebTransport.
https://socket.io
744 stars 355 forks source link

Remove GitHub URL from dependencies #358

Closed achingbrain closed 9 years ago

achingbrain commented 10 years ago

Right now engine.io-client has a GitHub URL as a dependency for the xmlhttprequest module. Looking into the tarball it references, it's version 1.5.0 of the module.

This version has been on NPM since 2012 and 1.6.0 was published last year.

Is there any chance of using the NPM version instead of the GitHub tarball?

luscus commented 10 years ago

related to my issue: https://github.com/Automattic/engine.io-client/issues/348

this will be fixed in the next release.

jfbibeau commented 9 years ago

Any update?

defunctzombie commented 9 years ago

@rase- We are currently using your branch of xmlhttprequest. Have those features been accepted upstream?

rase- commented 9 years ago

@defunctzombie not yet. The plan is to get them upstream either to the original repo or the learnboost fork, or maybe to a separate module before the next release.

tomrogers3 commented 9 years ago

+1. Can you please advise when will the next release be that will address this? Thanks

iostat42 commented 9 years ago

+1

frostme commented 9 years ago

there are currently these issue #348 , #384 on this same issue. Is there any update to this?

metamatt commented 9 years ago

I dug around a bit in the commit history for the various forks of xmlhttprequest. (Probably none of this is news to SIO/EIO contributors but is new to me and maybe other bystanders.) https://github.com/driverdan/node-XMLHttpRequest/commits/master is the original, it was forked at 9f926de into https://github.com/LearnBoost/node-XMLHttpRequest/commits/master which got 4 additional commits -- these changes do not look huge -- then that was forked at https://github.com/rase-/node-XMLHttpRequest/commits/add/ssl-support which got 2 additional commits to add SSL support, which is presumably what EIO/SIO want. In the meantime, work on the original repo has continued and it has ~25 commits the forks don't.

IMHO it would be great if one of the following happened:

ppitonak commented 9 years ago

It would be fine to fix because tools like NSP ignore dependencies with links:

$ nsp audit-package
Warning: Git dependency [xmlhttprequest@https://github.com/rase-/node-XMLHttpRequest/archive/a6b6f2.tar.gz] ignored.
Warning: Git dependency [global@https://github.com/component/global/archive/v2.0.1.tar.gz] ignored.
No vulnerable modules found
mdvorak commented 9 years ago

This is showstopper, since with absolute github.com reference, it ignores .npmrc settings and therefore it is impossible to clone this behind corporate proxy.

We are using Artifactory as npm mirror and don't have direct access to the internet. Only access is via proxy, with requires NTLM authentication. This can be bypassed by cntlm project, but reason we have artifactory is that we don't have to. This change breaks the builds.

sparty02 commented 9 years ago

+1 the absolute reference to the github 'dependency' is a real bummer. Could @rase- use npm's scoped packages to trivially publish this fork of xmlhttprequest separate from the original?

The dependency on xmlhttprequest could then be changed to something like:

"dependencies": {
    "@rase-/xmlhttprequest": "1.5.0"
  },

Something similar could be done with the dependency on the global module.

jskrzypek commented 9 years ago

@sparty02 the @rase- fork is already on npm as xmlhttprequest-ssl and this is committed in the package of engine.io-client however there were several errors with the package as committed and released by @rauchg on the 9th. I just submitted a PR now to fix, #401