RequireJS and angular

[araymer]

So this bit of code:

var _fs, jszip;
if(typeof JSZip !== 'undefined') jszip = JSZip;
if (typeof exports !== 'undefined') {
    if (typeof module !== 'undefined' && module.exports) {
        if(has_buf && typeof jszip === 'undefined') jszip = require('js'+'zip');
        if(typeof jszip === 'undefined') jszip = require('./js'+'zip').JSZip;
        _fs = require('f'+'s');
    }
}

Does not take into account if you are using requireJS without node. So you have modularity, the first if statement will not assign JSZip, but you're not using node, so the second if statement will not require properly, either. I suggest adding a default jszip = require('jszip') or changing the if/else logic a bit to allow for this.

So, maybe:

var _fs, jszip;
if(typeof JSZip !== 'undefined') jszip = JSZip;
else if (typeof exports !== 'undefined') {
    if (typeof module !== 'undefined' && module.exports) {
        if(has_buf && typeof jszip === 'undefined') jszip = require('js'+'zip');
        if(typeof jszip === 'undefined') jszip = require('./js'+'zip').JSZip;
        _fs = require('f'+'s');
    }
}
else
    jszip = require('js'+'zip');

[pietersv]

Thanks! This seems related to #17 also reported this morning.

[pietersv]

Referred to main branch SheetJS#339

[pietersv]

@araymer Might you have a test case I can use to verify? Even a snippet is helpful, need not be a complete mocha test.

Any suggestion why strings were broken, e.g. './js'+'zip' vs simply jszip? It looks like it was added to /bits/21_ziputils.js on 7/28/14 with this commit message.

version bump 0.7.8: docs, defaults, details

• browser shim updated (h/t @wintersm for discovering this)
• smart tag ignores (h/t @lostinplace)
• sheet_to_row_object_array bugfix (fixes Fix module error #80, h/t @ChrisBurkeBSD)
• README improved
• baltic and vietnamese codepages: updated codepage to 1.3.4
• iOS Numbers can handle inline strings -> disabling SST by default
• avoid Buffer accessors (see Buffer: implement a real readInt32 method nodejs/node-v0.x-archive#7809)
• caching certain hot regexes

[araymer]

Honestly, I have no idea why the strings weren't written contiguously for the require methods. I kept it as such out of caution. In my mind, it seems like it shouldn't make a difference.

As for a test case: I don't, sorry. I made the change on the project I was working on and just kinda went with it because it seemed to work fine (we were using bower, requireJS and angularJS with a .net backend. So, no node support beyond the build, but still needed modularity.)

[ralekna]

I guess that this was an optimization to avoid loading of jszip if it is included into document already. The problem is that module loaders like System.js parses such files statically to find dependencies but they don't do event primitive code evaluation inside require statements.

Fixing this would be very helpful.

[mattbasta]

I'd be happy to make a pull request that concatenates these strings, if it would be accepted.

[pietersv]

Are you able to run the test suite?

[mattbasta]

@pietersv Yes, this is the final output when I run make test:

This is for a clean clone of the repo.

[pietersv]

@mattbasta That's awesome. May I ask how you were able to run these, what Node version? On a clean clone of the root project on Node 4.1.1 I get 937 failed tests.

[mattbasta]

We are using 0.10. I used the instructions in the readme.