mikecousins / react-pdf-js

A React component to wrap PDF.js
https://react-pdf.cousins.ai
MIT License
724 stars 150 forks source link

Error in node_modules #60

Closed romax94 closed 6 years ago

romax94 commented 6 years ago

`ERROR in ./node_modules/react-pdf-js/lib/Pdf.js 02/02/2018 15:00:49Module not found: Error: Can’t resolve ‘pdfjs-dist/build/pdf.combined’ in ‘/app/node_modules/react-pdf-js/lib’ 02/02/2018 15:00:49 @ ./node_modules/react-pdf-js/lib/Pdf.js 49:0-40 02/02/2018 15:00:49 @ ./node_modules/react-pdf-js/lib/index.js 02/02/2018 15:00:49 @ ./src/components/serviceHistory/index.js 02/02/2018 15:00:49 @ ./src/containers/adverts/index.js 02/02/2018 15:00:49 @ ./src/containers/main/index.js 02/02/2018 15:00:49 @ ./src/index.js 02/02/2018 15:00:49 @ multi ./src/index.js

Driptap commented 6 years ago

I had this problem too, building v3.0.6 with the previously used version of pdfjs-dist works (2.0.203). I'm guessing there is some problem with that.

Driptap commented 6 years ago

Ok no - it's a peer dependencies. Make sure you have the correct pdfjs-dist version in your package.json:

"pdfjs-dist": "2.0.303", "react-pdf-js": "3.0.6",

romax94 commented 6 years ago

@Driptap issue still persist package json - web - visual studio code sell my car - tootle - google chrome

Driptap commented 6 years ago

Try manually deleting the react-pdf-js & pdfjs-dist folders from node modules, then installing the correct versions from the command line:

npm i -S react-pdf-js@3.0.6
npm i -S pdfjs-dist@2.0.303

I left the caret in by mistake on the previous comment, it still grabs latest version of pdfjs-dist

mikecousins commented 6 years ago

I'd remove the pdfjs-dist from your package.config and let this package handle it.

romax94 commented 6 years ago

@Driptap Thanks.

sebastianmenge commented 6 years ago

What is the solution for the problem now? When letting react-pdf-js handle it, thats exactly when i'm getting the error and i can't ged rid of it.

Driptap commented 6 years ago

Letting the package handle it seems to results in a newer version of pdfjs-dist (2.0.328) to be installed - this seems to be causing the Module Not Found error.

@sebastianmenge For now you should be able to install the correct version by adding "pdfjs-dist": "2.0.303" to your package.json, removing pdfjs-dist from node_modules and reinstalling.

@mikecousins maybe it is worth specifying the exact version of pdfjs-dist in react-pdf-js's package.json?

sebastianmenge commented 6 years ago

thanks @Driptap for clearing it up. i was just confused by the answer and thought the way to go would be not putting the pdfjs-dist in the package. working now.

romax94 commented 6 years ago

@mikecousins When there will be release with corrected bugs?

netpoetica commented 6 years ago

Currently, in my project I had to add

    "pdfjs-dist": "2.0.139",
    "react-pdf-js": "3.0.0",

to this specific version of pdfjs-dist. It appears that the newer version of pdfjs-dist is missing pdf.combined JS file, which even the master branch of this repo depends on still.

Which, is probably a good thing, because that file (old version) injects an override of Object.defineProperty which does not work as expected: https://raw.githubusercontent.com/mozilla/pdfjs-dist/846bbae9bcb9ce06ca3d04da9e99beecb9900e82/build/pdf.combined.js

Object.defineProperty = function objectDefineProperty(obj, name, def) {
      delete obj[name];
      if ('get' in def) {
        obj.__defineGetter__(name, def['get']);
      }
      if ('set' in def) {
        obj.__defineSetter__(name, def['set']);
      }
      if ('value' in def) {
        obj.__defineSetter__(name, function objectDefinePropertySetter(value) {
          this.__defineGetter__(name, function objectDefinePropertyGetter() {
            return value;
          });
          return value;
        });
        obj[name] = def.value;
      }
    };

This library should use the non "combined" version of this file, no?

netpoetica commented 6 years ago

@Driptap I can confirm that the Object.definePropert tomfoolery is removed in this version of pdfjs-dist (https://raw.githubusercontent.com/mozilla/pdfjs-dist/d90724150fdff57ba76c25e2f3167d12105ba2a1/build/pdf.combined.js, find "defineProperty =" missing). However, if I have dependencies

  "dependencies": {
    "pdfjs-dist": "2.0.303",
    "react-pdf-js": "3.0.6"
  },

I still end up with the new version of pdfjs-dist, which is incompatible, in the local node_modules/ of react-pdf-js


➜  myproj git:(master) ✗ npm list pdfjs-dist
myproj@0.6.26 /Users/rosenbek/myproj
├── pdfjs-dist@2.0.303 
└─┬ react-pdf-js@3.0.6
  └── pdfjs-dist@2.0.332 

and version 2.0.332 is missing pdf.combined.js.

The PR above should fix this problem because it is in the dependencies of react-pdf-js that this issue arises (https://github.com/mikecousins/react-pdf-js/pull/62/files), however, it still stands that isn't it a bad idea to use the pdf.combined.js file? It seems like so much bloat is in there? And like I said, that polyfill/hijacking of Object.defineProperty seems to behave differently from native Object.defineProperty.

mikecousins commented 6 years ago

I have merged and released the change to hardcode the version to the old one.

I like this talk about switching away from pdf.combined but I'm unsure of the ramifications of it.

MichaelDeBoey commented 6 years ago

@mikecousins We should indeed get rid of the pdf.combined 🙂 Since the provided polyfills won't be included anymore, that would be a major version bump.