Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alasql in browserify setup in the browser #583

Closed
nowzig opened this issue Feb 3, 2016 · 34 comments
Closed

Alasql in browserify setup in the browser #583

nowzig opened this issue Feb 3, 2016 · 34 comments

Comments

@nowzig
Copy link

nowzig commented Feb 3, 2016

When doing importing or exporting of files (e.g. Excel) there is always an error like Uncaught TypeError: fs.writeFileSync is not a function

I use alasql with browserify in the browser.

I think it can be fixed by changing the checks like
if (typeof exports === 'object')
to
if (typeof exports === 'object' && process.browser !== true)

Maybe there is a better solution?

@mathiasrw
Copy link
Member

Good point. The problem is found here - and your suggestion to a change makes a lot of sense.

In #562 we are looking into making a central place to check for enviroment, so when its working we will replace the if (typeof exports === 'object') with if (isNode()) so that we also support browserify

Would it be ok if I get back to you when we need to get it tested in browserify?

@mathiasrw mathiasrw self-assigned this Feb 3, 2016
@agershun
Copy link
Member

agershun commented Feb 3, 2016

yes, agree.

NB: It is better to check if the process object availabe before: (typeof process === "object" && process.browser !== true)

@nowzig
Copy link
Author

nowzig commented Feb 3, 2016

Great, that you are already working on a solution based on a central environment detection.
I would be happy to test it.

Yeah, checking if the process object is available makes sense.

@nickdeis
Copy link
Contributor

nickdeis commented Feb 4, 2016

NB: You can exclude modules in browserify config by marking them as "external resources" or by pointing to an empty js file and aliasing it to that file.
@mathiasrw + @agershun

Using isNode won't fix this, as both Webpack and Browserify use the AST and just pick up whatever has "require".
For Webpack I finally have a better solution this script-loader (and have been meaning to make a gist of it), but the other choice is to provide a build with all the requires stripped out.

UPDATE: Found this

@nickdeis
Copy link
Contributor

nickdeis commented Feb 4, 2016

Exclude works well enough.

@mathiasrw
Copy link
Member

@nowzig Could you please retry again with the last version of alasql.js?

@nickdeis If we detect if its node or browser the right way, dont you think we are able to avoid forcing people to use exclude on packing with browerify?

@nowzig
Copy link
Author

nowzig commented Feb 4, 2016

First of all thank you for the fast corrections.

But now I get the error Uncaught Error: Please include a Promise library. The error comes from here

Could it be because the check is for object but window.Promise is of type function?

UPDATE: If I change the check to if(typeof utils.global.Promise === "function") I still get the same Uncaught TypeError: _fs.writeFileSync is not a function as above.

@mathiasrw
Copy link
Member

Wops.... please try this one instread: https://github.com/agershun/alasql/blob/develop/dist/alasql.js

@nowzig
Copy link
Author

nowzig commented Feb 4, 2016

Thanks for the fix. The Promise error is gone, but now I get the following error: Uncaught TypeError: _fs.writeFileSync is not a function. This error comes from xlsx.js:11412

UPDATE: This seems to be due to the checking here

@nickdeis
Copy link
Contributor

nickdeis commented Feb 4, 2016

Yeah, about time I add isNode/isWorker/isBrowser.

@mathiasrw
Copy link
Member

@nowzig Please try again with https://github.com/agershun/alasql/blob/develop/dist/alasql.js

@nickdeis I have included it as util.isNode, util.isBrowser... Its not a function as we only need to find out once. I have changed your code from util.getGlobal() to utils.global. I was not sure if getting rid of the "get" part is a clever move, but when its not a function it felt strange to keep the ´get´

@nickdeis
Copy link
Contributor

nickdeis commented Feb 4, 2016

Should be fine for the most part. Kind of a fan of transitive values but that's the Java in me.

@mathiasrw
Copy link
Member

transitive?

@nickdeis
Copy link
Contributor

nickdeis commented Feb 4, 2016

Computed value. It's really only a valid pattern in a multithreaded environments since you always want to reinsure that the value is indeed correct.
Web Workers don't really count, since they use message passing, which is always thread safe (best W3C Spec ever done btw).

Even though I use stuff like Akka for multithreading, I still get punished for using fields sometimes.

@nowzig
Copy link
Author

nowzig commented Feb 5, 2016

So the Uncaught TypeError: _fs.writeFileSync is not a function error is solved.

But now I get Uncaught TypeError: Cannot read property 'write' of undefined here

If I want to read from a xlsx file I get Error: XLSX library is not attached in this line
Same with xls file but the error comes from here

@mathiasrw
Copy link
Member

Ok. We are slowly getting there...

AlaSQL uses js-xlsx library to read and export Excel files. Please include https://github.com/SheetJS/js-xlsx/blob/master/dist/xlsx.min.js

update: and im working on Uncaught TypeError: Cannot read property 'write' of undefined being replaced with a more informative error message...

@nowzig
Copy link
Author

nowzig commented Feb 5, 2016

Step by step it's getting better ;)

I do now a global.XLSX=require('xlsx') in my code. Now reading a xlsx file is working,but for xls files the xls library is missing. So I guess i have to do a global.XLS=require('xls')

For exporting a xlsx File I get Uncaught TypeError: saveAs is not a function here

I am also wondering wether there is a solution that alasql requires the libraries it depends on when it is used via browserify

@mathiasrw
Copy link
Member

Oh boy... We are getting a lot of enviroment specifik things here. Lets hope we are solving more problems than we are introducing :)

If you can tell me how I can find out that its running in a browersify setup, then we can navigate around it...

@mathiasrw mathiasrw reopened this Feb 5, 2016
@mathiasrw
Copy link
Member

Woops

@mathiasrw mathiasrw changed the title 'fs' is not defined when alasql is used with browserify in the browser Alasql in browserify setup in the browser Feb 5, 2016
@mathiasrw
Copy link
Member

@nowzig

Could you do me a favor and run this in a browserify setup and tell me what is being printed to the log?

var utils = {}

var getGlobal = function(){
  try {
    return Function('return this')();

  }catch(e){  
    //If Content Security Policy
    var global =  self || window || global;

    if(global){
        return global;
    } else {
        throw new Error('Unable to locate global object');
    }
  }
}
utils.global = getGlobal();

var isNativeFunction = utils.isNativeFunction = function(fn){
  return typeof fn === "function" && !!~(fn.toString().indexOf("[native code]"));
}

var isBrowser = function(){
  try{
    return utils.isNativeFunction(utils.global.location.reload);
  }catch(e){
    return false;
  }
}
utils.isBrowser = isBrowser();

var isBrowserify = function(){
  return utils.isBrowser && typeof exports === 'object';
}
utils.isBrowserify = isBrowserify();

console.log(JSON.stringify(utils, null, 4));

Update: updated code for errors

@nowzig
Copy link
Author

nowzig commented Feb 5, 2016

I get a Uncaught TypeError: Converting circular structure to JSON

But if I am logging just the utils object I get this:
global: Window isBrowser: true isNativeFunction: function (fn)

Maybe that is the information you want.

UPDATE: I think it is possible to detect a browserify setup via process.browser as suggested in the first post. See this

UPDATE2: I corrected your code a bit, that the isBrowserify function is assiciated to the utils object, now the output of utils is:
global: Window isBrowser: true isBrowserify: true isNativeFunction: (fn)

@mathiasrw
Copy link
Member

Ohh, I see - sorry for that.

Cool. I think we have a way to solve it then

@mathiasrw
Copy link
Member

@nowzig Ok - I cant figure out if browserify will be able to figure out that it should require XLSX - but try to run the latest https://github.com/agershun/alasql/blob/develop/dist/alasql.js where I let the utils.isBrowserify load XLSX same way as node, but handle saving a file same way as a browser...

Update:

process.browser

Ohh - only saw this now... changed the identification.

@nowzig
Copy link
Author

nowzig commented Feb 8, 2016

Great, the isBrowserify function seems to work. Thank you.

But now the checkings for some functions need to use this function:
The saveAs function must be defined when browserify is used, this should be done here I think.
Also here and here the isBrowserify function should be added.

@mathiasrw
Copy link
Member

Your suggested changes have been implemented in the latest build: https://github.com/agershun/alasql/blob/develop/dist/alasql.js

@nowzig
Copy link
Author

nowzig commented Feb 9, 2016

Thanks for all your help and work.
It seems like yesterday I was a little too early saying the isBrowserify detection works. I did some testing and I think the function should be:
var isBrowserify = function(){ return utils.isBrowser && process && process.browser }

Because the process object is not a child of the window object.

One other thing, in 97saveas.js the check should be if(utils.isBrowser && !utils.isWebWorker) { because otherwise the saveAs function does not get defined when used with browserify.

@mathiasrw
Copy link
Member

Great. The changes have been implemented in the latest build: https://github.com/agershun/alasql/blob/develop/dist/alasql.js

@nowzig
Copy link
Author

nowzig commented Feb 9, 2016

It is now working perfectly. Thank you very much.

@nowzig nowzig closed this as completed Feb 9, 2016
@agershun
Copy link
Member

agershun commented Feb 9, 2016

Question: Is this statement works correct if process variable is not defined?

return utils.isBrowser && process && process.browser;

I think we should,to rewrite it with typeof guard. Is this correct?

return utils.isBrowser && (typeof process !== "undefined") && process.browser;

@mathiasrw
Copy link
Member

I think we should,to rewrite it with typeof guard

I will rewrite.

Update: is typeof process === "object" better?

@agershun
Copy link
Member

agershun commented Feb 9, 2016

👍

@mathiasrw
Copy link
Member

@agershun Did you see my question at 9045286#commitcomment-15945734 ?

@agershun
Copy link
Member

agershun commented Feb 9, 2016

Sorry for delay, just answered.

@mathiasrw
Copy link
Member

:) 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants