Empty object when browserified

[nhagen]

I have a simple wrapper module that starts with:

var EventEmitter = require('events').EventEmitter;
var util = require('util');
var swagger = require('swagger-client');

console.log(swagger) // { } in browser, { ... normal stuff ... } in repl

// Bunch of stuff below

As mentioned by the log comment, I get different values in REPL and the browser. I can use it in the REPL but not in the browser. Should this client not work with browserify?

[nhagen]

It looks like browserify doesn't like the var e = exports pattern.

Finding and replacing (being careful with the whitespace!)
var e => // var e
e. => exports.
(e. => (exports.
fixed this issue, but causes this problem:

`Uncaught ReferenceError: CookieAccessInfo is not defined request.js:357``

CookieAccessInfo is exported from this dependency, which is required everywhere except request.js:

https://www.npmjs.com/package/cookiejar

Adding
var CookieAccessInfo = require('node-cookiejar').CookieAccessInfo to request.js resolves that.

and finally, there seems to be a bug with shred near line 86 in lib/shred/response.js where there is this code:

  78   var chunkBuffers = [];
  79   var dataLength = 0;
  80   raw.on("data", function(chunk) {
  81     chunkBuffers.push(chunk);
  82     dataLength += chunk.length;
  83   });
  84   raw.on("end", function() {
  85     var body;
  86     if (typeof Buffer !== 'undefined') {
  87       // Just concatinate into a string
  88       body = chunkBuffers.join('');
  89     } else {
  90       // Initialize new buffer and add the chunks one-at-a-time.
  91       body = new Buffer(dataLength);
  92       console.log(chunkBuffers);
  93       for (var i = 0, pos = 0; i < chunkBuffers.length; i++) {
  94         chunkBuffers[i].copy(body, pos);
  95         pos += chunkBuffers[i].length;
  96       }
  97     }

Here, the http response is expected to be a buffer, but (at least in my case), was a string. I cut that out to get this:

  78   var chunkBuffers = [];
  79   var dataLength = 0;
  80   raw.on("data", function(chunk) {
  81     chunkBuffers.push(chunk);
  82     dataLength += chunk.length;
  83   });
  84   raw.on("end", function() {
  85     var body = chunkBuffers.join('');
  86

and now it works! Yay!

So this is a really hacky solution because I'm editing stuff in node_modules. This could be resolved with a pull request to this project, but the changes I made would break this in browsers (without browserify). However, the var = exports pattern seems to be incompatible with browserify.

I would propose that the 'correct' solution to this is for swagger-client to be a fully nodejs/commonjs compliant module (i honestly can't find specific documentation saying it isn't--maybe this is actually a bug with browserify?) by getting rid of all of the context checking, use requires at the top, use exports like normal, and then use something like browserify swagger.js --standalone swagger.bundle.js to provide a single bundle with all dependencies included. There also seems to be issues with the shred library, but it might just be that what's in there is outdated? This might explain why I didn't get those errors in REPL, if REPL is not using the provided shred.js files. I haven't looked into that mystery yet.

This is a tall order I think, so I'd like to get feedback about what can actually be done here, or if I'm just misunderstanding something about the codebase.

[fehguy]

Hi Nathan, thank you for the details on this. A couple thoughts:

1. the exports pattern you're using makes sense and would be good for a PR if you could
2. the shred version we're using is behind their trunk. The shred folks have taken a new approach to the library, so we may continue with a fork of it, and continue to patch it.
3. is cookiejar included in shred.bundle, which you're not pulling in via browserify?
4. i'm not sure how to dig into the buffer issue that you've come up with. I do believe you, but have never hit that before.

We have recently updated the test framework and should be able to have good mocha tests around each of the above issues. That would be the first step.

[nhagen]

Yeah, I think a PR going for the export pattern is managable. I'll look at it this week.

Cookiejar seems to work from the shred bundle (iirc, AccessCookieInfo is defined in the bundle, but not in the unbundled lib file where the error occurs). It also works when I load the module in REPL. I think this might work because node references the installed shred module, not the included bundle or lib files. That's just off the top of my head though.

Browserify, however, doesn't seem to be pulling anything from shred.bundle and I'm not sure if its possible to make that happen.

I did not look into the buffer issue, just bulldozed through it. That one seems really weird in retrospect. Some other change I made must have caused it.

[nhagen]

Looks like the fixes I have in mind are about 4 of the points listed in #196. Part of changing the export/require pattern would be made much easier/cleaner by resolving the issues with shred.

What is the difference between the shred http client and the jquery client? Why not just depend on jquery?

Actually, I guess I should have checked first. Doesn't look like npm's jquery is compatible for server-side use, which makes sense.

How difficult would it be upgrade to mainline shred?

[fehguy]

Hi @nhagen there are two js library dependencies for the purposes of compatibility. The jQuery library is used for older versions of IE automatically. It is also used for file uploads as that's not supported in shred.

The problem with jQuery is that it was causing trouble when deploying to node.

[nhagen]

Is the IE issue with shred related to shred or the underlying http module? And even if shred were browserified, swagger-js would still need jquery for file uploads?

[fehguy]

much deeper than that. It's in IE's ability to handle functions in object declarations. It was a big problem for IE7, 8 compatibility

[whitlockjc]

This should be fixed in #279. In fact, that commit also uses Browserify to build the browser binaries.

[whitlockjc]

I just merged #279 so browserify support should be available now.