-
-
Notifications
You must be signed in to change notification settings - Fork 255
Handling scripts that assume nodejs modules are available if in CommonJS environment #79
Comments
Perhaps try using |
I actually tried that first. Both the npm and github versions work the On Fri, Dec 5, 2014, 2:58 AM Guy Bedford notifications@github.com wrote:
|
along the lines of https://stackoverflow.com/questions/23038483/how-to-detect-when-browserify-is-being-run ... maybe js-xls should just use having taken a slight look in https://github.com/SheetJS/js-xls/blob/master/xls.js it seems they don't use browserify, so checking for This code looks like once being a global in the browser got shoved in the ability to run in node rather quick and dirty .. not regarding practices like https://github.com/substack/browserify-handbook or https://gist.github.com/defunctzombie/4339901 I know all this doesn't solve your current problem, it's just some things I noticed. edit: I'm guessing if they'd change their module format from global to CommonJS they'd encounter the same problem ;) |
taking a slightly deeper look in the code base, https://github.com/SheetJS/js-xls/blob/master/dist/xls.js#L1687 seems the only place where the above mentioned error could happen. I didn't find out though, who or what is calling the public
|
Apologies for the delayed response on this. I see it is a global actually - yes loading from GitHub would make sense then and you can do:
Just let me know if you want to do a PR on the override or if you need help with it at all. |
I didn't use SheetJS myself, but @tauren, isn't it true that the error (edit) maybe you should ad nodelibs package to your app in that case (at least until the loader supports conditionally loading). |
@guybedford Thanks! I didn't realize that JSPM could work that way. It worked perfectly with your suggested override, Bravo! I'll create a PR. @andreasgrimm Thanks for your investigation. I do believe that when I was experiencing the |
@tauren glad to hear that! That is actually one of the main use cases for overrides! Setting how to interpret a module, as the module environment the module is supposed runs in is what jspm needs to know. |
Just for clarification .. @tauren you said that "Both the root xls.js and the dist/xls.js try to use node dependencies when used in the browser via JSPM". (edit) I guess I'm missing something here. |
@andreasgrimm I assume the code does environment detection something like: if (typeof require != 'undefined') {
var fs = require('fs');
} By forcing jspm to treat it as a global and not CommonJS (which it would be detected as by default in jspm), it would never run that code. |
that's exactly what I mean and what confused me, because the code actually is if(fs === undefined) fs = require('fs'); and that code gets executed depending on external data (input). |
@andreasgrimm I believe that line of code can only be executed if you use |
@tauren That'd make sense. But then on the other hand I wonder why you get that error when the module doesn't get interpreted as global but as CommonJS instead. Because you didn't call that function explicitly but just loaded the module, right? |
I guess it's because in CommonJS "mode" (as in ES6 and AMD as well) at startup time all the code gets parsed for @guybedford Can you confirm this? Sorry if it's too obvious. In that case adding |
@andreasgrimm I believe you are correct. In "global" mode, pre-parsing the file for |
Great to hear these issues are making sense without further explanation Will certainly bear this in mind - I'm hoping to write more articles about On 12 December 2014 at 10:15, Tauren Mills notifications@github.com wrote:
|
The
xls.js
script found at https://github.com/SheetJS/js-xls assumes it can usefs
and other node.js modules if it is within a CommonJS environment. This means that running it within a browser environment with JSPM is throwing errors such asfs.js not found
. The readme indicates the script supports running in a browser and will not load nodejs modules or extra functionality such ascpexcel
.Here are some code snippets:
It seems to me the code would have to be changed to work within JSPM and that simple overrides will not solve this problem. Is this true, and if not, any ideas how to make it work?
The text was updated successfully, but these errors were encountered: