mholt / PapaParse

Fast and powerful CSV (delimited text) parser that gracefully handles large files and malformed input
http://PapaParse.com
MIT License
12.54k stars 1.15k forks source link

Is PapaParse 4.5.0 supported in browser? #517

Closed thaoula closed 6 years ago

thaoula commented 6 years ago

Hi Guys,

Is PapaParse 4.5.0 still supported in browser?

We get the following error when building using Angular CLI 6.0.5 -

ERROR in ./node_modules/papaparse/papaparse.min.js 2018-06-04T12:54:43.4234387Z Module not found: Error: Can't resolve 'stream' in 'node_modules/papaparse'

Downgrading to version 4.4.0 works fine.

Regards, Tarek

pokoli commented 6 years ago

Hi Tarek,

This code was introduced on https://github.com/mholt/PapaParse/commit/392408edcc4d03d6e30aa80fffa1982231cd1303

It is expected that this code is not run on the browser so the require should not be executed on any browser version and it should never fail.

When do you get this error?

timbru31 commented 6 years ago

I'm getting the same error when upgrading to v4.5.0. Webpack throws this error when compiling the Angular app.

pokoli commented 6 years ago

Sorry but I'm not so much familiar with webpack. Is there any way to make it skip this kind of errors or to tell that this code will be never executed on browser?

timbru31 commented 6 years ago

Seems this might be an issue with Angular-CLI v6, too: https://github.com/angular/angular-cli/issues/10681 I'll keep debugging and update my response if I find any clues 🔍

pokoli commented 6 years ago

@timbru31 If you find any workaround let us know and will try to include it in a bug fix release if possible

jefbarn commented 6 years ago

workaround is add readable-stream module as a substitute for stream. Alias in tsconfig.json:

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "stream": [
        "node_modules/readable-stream"
      ]
    },
joshtune commented 6 years ago

@jefbarn thanks for the tip but that didn't work for me. In return I get an error like this

Error: Can't resolve '.... /node_modules/readable-stream' in '... /node_modules/papaparse'

manish2237 commented 6 years ago

This is the actual workaround...

"paths": { "stream": [ "../node_modules/readable-stream" ] },

dawidrylko commented 6 years ago

agree with @manish2237

pokoli commented 6 years ago

So we agree that we can close this issue with the proposed solution?

timbru31 commented 6 years ago

While this might solve the issue, the underlying change is in Angular CLI 6+ (see https://github.com/angular/angular-cli/issues/9827#issuecomment-369578814) As a workaround without an extra dependency, you can patch the webpack config like this: https://gist.github.com/niespodd/1fa82da6f8c901d1c33d2fcbb762947d

pokoli commented 6 years ago

I'm closing this issue as I understand that we don't have to do anything in PapaParse as this is an Angular issue and several workarrounds have been proposed already.

Thanks to all who contributed to this issue.

jefbarn commented 6 years ago

@pokoli I would say that the underling issue is that PapaParse is trying to use non-browser features in a browser context. While most front-end packaging systems can work around this, the Angular team took a hard line and is refusing to support loading node features into the browser. I think the correct answer is to have separate node and browser builds if the intent is not to support streams in the browser, or to specifically load a stream library that can be used in either context (readable-stream).

Rutulpatel7077 commented 6 years ago

I faced same error in React Native! any solution ?

Lingyis commented 6 years ago

Same here, I have the same error in React Native

Rutulpatel7077 commented 6 years ago

@Lingyis I have solution for this!.

Install this stream npm package and please follow the steps.

Restart you react-native server or Expo client server.

manifoldhiker commented 6 years ago

@Rutulpatel7077 unfortunately, this workaround is not tracked by git. @pokoli 4.5.0 became incompatible with react-native. I hoped this library not to have any platform specific dependencies.

Rutulpatel7077 commented 6 years ago

@mitutee Instead of using node_module you can put all files of PapaParse in your project and use papaparse from there

Lingyis commented 6 years ago

my solution is to just downgrade to 4.4 like those before me mentioned. for me it's perfectly fine, i only use a very limited set of features.

Rutulpatel7077 commented 6 years ago

@Lingyis Good to know! Otherwise, Install stream as I mentioned above and copy the PapaParse folder from node_modules to your root of the project and import Papa from 'from you folder path'

I can run every feature of PapaParse.

pokoli commented 6 years ago

@mitutee indeed the dependency is optional and not required to run on other plataforms.

The issue is that react-native does not detect it as optional and complains about it.

btakita commented 6 years ago

I'm having the same issue with sapper & rollup. I'd rather not use the "workaround" because 'stream' is not necessary in the browser.

Another workaround that could be used is to vendorize PapaParse. However, it would be great if dependencies on this project were able to be sorted out.

arxpoetica commented 6 years ago

Having the same issue as @btakita and hoping for PapaParse to resolve its dependency issues. To be clear, it appears, as @jefbarn said above:

I would say that the underling issue is that PapaParse is trying to use non-browser features in a browser context. [.i.e., the stream module, which is native only to Node]

@pokoli can this be fixed in PapaParse itself?

pokoli commented 6 years ago

@arxpoetica how do you propose to fix it?

For me there is no dependency problem as the code only uses then library when available, so if you are on the browser and the library is not available it won't use it.

jefbarn commented 6 years ago

@pokoli the problem is that modern code bundling tools don’t ignore the offending require and instead report an error. A simple fix could be to refactor the code slightly to have a browser build that omits the require and list it under the browser tag in package.json

pokoli commented 6 years ago

@jefbarn if I understand correctly this mean having two PapaParse packages? Or a single one?

I'm happy to see a PR that implements this refactor and test it. It should not be so hard as there is only one line with a ofending package.

jefbarn commented 6 years ago

It would be one npm package with two different .js entry points depending on context. I’ll work on a PR tomorrow.

jefbarn commented 6 years ago

See PR #580