minutemailer / react-popup

React popup component
http://minutemailer.github.io/react-popup/
MIT License
208 stars 71 forks source link

Can't resolve 'keymaster' #43

Closed sbmthakur closed 6 years ago

sbmthakur commented 6 years ago

After installing I receive the following error:

Failed to compile.

./node_modules/react-popup/dist/Popup.react.js
Module not found: Can't resolve 'keymaster' in '/home/shubham/mystuff/ttt_form/node_modules/react-popup/dist'

I tried re-installing react-popup, but still keymaster wasn't downloaded. So, I installed keymaster separately and things worked.

tbleckert commented 6 years ago

Yes, keymaster is listed as a peerDependency. So when you install react-popup you will get a warning if keymaster doesn't exist in your project and tells you to install it.

I will however make keymaster optional in the next release. If you have it, we'll use it, otherwise not. You can read more about this here https://github.com/minutemailer/react-popup/issues/41#issuecomment-353476693 . And if you're not familiar with the concept of peerDependencies you can read about it here https://nodejs.org/en/blog/npm/peer-dependencies/ .

Finally, if you believe I'm wrong for using peerDependency for keymaster we can reopen the issue and I'll happily discuss it. All help is appreciated. Thanks for using react popup!

sbmthakur commented 6 years ago

Okay, thanks for the update.

z-vr commented 6 years ago

@tbleckert because keymaster is required with var _keymaster = require('keymaster'); in dist (import key from 'keymaster'; in src), it means that any react app which does not have it installed as dependency will fail to compile.

Failed to compile.

./node_modules/react-popup/dist/Popup.react.js
Module not found: Can't resolve 'keymaster' in 'react-app/node_modules/react-popup/dist'
tbleckert commented 6 years ago

Yes, that's true @z-vr and was the intention (same thing if you install this package without having react installed), so maybe it should not be a peer dependency or it should be optional. Or make it optional.

Still, you get a warning when you update or install the package.

z-vr commented 6 years ago

@tbleckert but what's the point of making it peer-dependency if it's not going to work without it? How is it different from making it a proper dependency?

tbleckert commented 6 years ago

Hm, this made me think @z-vr . In npm2 dependencies weren't flat. So if one package had a dependency of let's say keymaster@1.0 and another package had keymaster@2.0 as a dependency. In this case both versions of keymaster would be installed and in your source code. Like this:

reactApp
|- node_modules
   |- react-popup
   |  |- node_modules
   |     |- keymaster
   |- keymaster

peerDependencies fixed this, so packages would use peerDependencies and the site/app/whatever would use dependencies resulting in a clean build with only 1 version of each package. peerDependencies was also automatically installed in npm2.

But npm3 introduced a flat directory, meaning no more node_modules inside modules. So I'm not really sure what the main purpose of peerDependencies are now.

z-vr commented 6 years ago

@tbleckert yeah I'm not sure either,I used to to detect whether a dependency installed, with

let dep
try {
    dep = require('dep')
} catch (err) {
   console.warn('dep is not installed')
}
if (dep) // do something 

however, with import statement being static evaluation, this doesn't work any more.