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

Can't resolve 'keymaster' #43

Closed
sbmthakur opened this issue Jan 7, 2018 · 7 comments
Closed

Can't resolve 'keymaster' #43

sbmthakur opened this issue Jan 7, 2018 · 7 comments

Comments

@sbmthakur
Copy link

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
Copy link
Contributor

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 #41 (comment) . 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
Copy link
Author

Okay, thanks for the update.

@z-vr
Copy link
Contributor

z-vr commented Jan 11, 2018

@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
Copy link
Contributor

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
Copy link
Contributor

z-vr commented Jan 11, 2018

@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
Copy link
Contributor

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
Copy link
Contributor

z-vr commented Jan 12, 2018

@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.

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

No branches or pull requests

3 participants