browserify support #455

Closed
tilgovi opened this Issue Mar 2, 2013 · 22 comments

10 participants

@tilgovi

While working on tilgovi/chromify last month I ran into an issue where the mime types file couldn't be read by request (because it wasn't included). At the time I hacked around it hard by bundling the mime types file through emscripten and a readFileSync that read from that.

With the 'browser' attribute of either request or the mime types module I bet we can preload the data so that it just works.

I'll look into it and come back with a PR over the weekend.

@mikeal
request member

we need to get a pull request in to the mime module.

@substack substack referenced this issue in substack/node-browserify Mar 21, 2013
Closed

Question: 'request' package supported with browserify? #332

@maxogden

bumping this with an update: https://twitter.com/maxogden/status/413057941185372160

tl;dr @mikeal said he's hoping to work on this over xmas break

@kyledrake

Did santa deliver on this one? Want to use it for NeoCities API client for node http://neocities.org/api

@maxogden

http://npmjs.org/xhr is the best bet right now, it got some love recently.

@substack

Some folks forked node-mime and took on browser-compatibility. It should be a trivial drop-in replacement: https://github.com/expressjs/mime-types

@mikeal
request member

I'll +1 a PR that swaps the mime library fro one that is browserifyable.

@eiriksm eiriksm referenced this issue Jun 25, 2014
Merged

New mime module #943

@eiriksm
request member

Hello.

So the issue referenced right above this comment should fix that mime problem and get request ready for being "browserified" without error. However, I ran into one additional issue while using request with browserify. I am pretty sure you guys have a better understanding as to how to proceed with it, but this is the temporary hack I added:

diff --git a/request.js b/request.js
index caa1394..001a380 100644
--- a/request.js
+++ b/request.js
@@ -716,7 +716,7 @@ Request.prototype.onResponse = function (response) {
     debug('response end', self.uri.href, response.statusCode, response.headers)
   });

-  if (response.connection.listeners('error').indexOf(self._parserErrorHandler) === -1) {
+  if (response.connection && response.connection.listeners('error').indexOf(self._parserErrorHandler) === -1) {
     response.connection.once('error', self._parserErrorHandler)
   }
   if (self._aborted) {
@@ -725,7 +725,7 @@ Request.prototype.onResponse = function (response) {
     return
   }
   if (self._paused) response.pause()
-  else response.resume()
+  else response.resume && response.resume()

As you might understand, response.resume and response.connection is undefined in the browser, and I have a feeling this might come from browserify-http or something like that. It's a lot of code to read through, so if this is really obvious to anyone, please chime in

(also if my temporary hack is the only way to go, and you feel like it's alright, I'll open another PR for it)

@mikeal
request member

why doesn't response have a resume() method? i would think that the stream shim would provide that?

@mikeal
request member

oh, also, can you put a comment on each of these lines saying that it is for browserify, cause i'm going to forget this entirely in the next refactor :)

@eiriksm
request member

Sorry if my question was not so obvious, but does that mean you want me to create a PR with those lines? (including comments of course)

My comment was also meant as a question (as you mention yourself) as to why this does not work. :)

But PR it is, if you want... :)

@mikeal
request member

I'm interested in why this is required but that doesn't mean I won't accept a workaround right now that gets us to a browserifyable request :)

@eiriksm
request member

I like (and agree with) the pragmatism :)

Pull request sent.

@mikeal mikeal closed this in #944 Jun 25, 2014
@mikemaccana

FYI current request 2.48.0 on npm still uses a form-data 0.1.4, which uses mime 1.2.11 that does content = fs.readFileSync(file, 'ascii') and fails on mime.js:55 when called from browserify.

@nylen nylen reopened this Nov 28, 2014
@eiriksm
request member

This was apparently introduced in 2.46 or something, when form-data started to be a dependency instead of an optional dependency. PR to form-data, maybe?

@nylen
request member

Ping @mikeal re bounty on this issue

See form-data/form-data#87, looks like it just needs some cleanup.

@eiriksm eiriksm referenced this issue in form-data/form-data Dec 5, 2014
Merged

Replace mime library. #95

@eiriksm
request member

Just tested replacing the mime library in form-data, and that fixes the issue. Sent a cleaned up PR just like the above mentioned.

@nylen nylen closed this in e165ee3 Dec 7, 2014
@eiriksm
request member

Can we publish a new version, by the way?

@tschaub

Also, can someone list the last known version to work with browserify? Until a new version is published, it would be good for folks who find this to be able to know what works.

@tschaub

It looks like 2.40.0 works with browserfiy. Though this doesn't provide HTTPS support. 2.46.0 should have HTTPS support in the browser (with #1173) but this has a transitive dependency on a non-browser friendly version of the mime module.

@nylen
request member

2.50.0 is out now. Let us know if you have any problems with it.

@zordius zordius added a commit to zordius/fluxex that referenced this issue Dec 10, 2014
@zordius zordius fix for browserify request ... ref: request/request#455 0de9ae1
@paulmand3l paulmand3l added a commit to paulmand3l/sparkjs that referenced this issue Feb 14, 2015
@paulmand3l paulmand3l Bumping request minor version
request/request#455 (comment)

Also, the browser-request library doesn't include the .on function used here https://github.com/spark/sparkjs/blob/master/lib/spark.js#L667, so trying to do any event subscribing in the browser just results in 'undefined is not a function' in a minified file, which isn't super helpful.
5c0cda0
@phillbaker phillbaker added a commit to phillbaker/digitalocean-node that referenced this issue Mar 18, 2016
@phillbaker phillbaker Add browserify and build commands for compiling browser friendly libr…
…ary.

Note this depends on request's built in support for being browser friendly: request/request#455.
83e87a4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment