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

stop using optional deps to fix webpack #53

Closed
BryanCrotaz opened this issue Aug 27, 2018 · 8 comments
Closed

stop using optional deps to fix webpack #53

BryanCrotaz opened this issue Aug 27, 2018 · 8 comments

Comments

@BryanCrotaz
Copy link

@BryanCrotaz BryanCrotaz commented Aug 27, 2018

ecc-jsbn and tweetnacl appear to not be optional

@StefanFeederle
Copy link

@StefanFeederle StefanFeederle commented Sep 4, 2018

My webpack build also fails because it's missing tweetnacl, bcrypt-pbkdf, jsbn and ecc-jsbn.

@Immortalin
Copy link

@Immortalin Immortalin commented Sep 30, 2018

Any luck getting this to run on browser?

@light24bulbs
Copy link
Contributor

@light24bulbs light24bulbs commented Oct 10, 2018

These packages being misclassified is causing compatibility issues with certain build systems. This is an issue.

@light24bulbs
Copy link
Contributor

@light24bulbs light24bulbs commented Oct 10, 2018

Easy fix. Opened a PR #55

@arekinath
Copy link
Contributor

@arekinath arekinath commented Oct 10, 2018

Do you have a specific error message that might be helpful to figure out what's going on that causes it not to work? The deps ecc-jsbn, tweetnacl etc that are in optionalDependencies are there because sshpk very carefully only calls require() on them if you use the actual functions that depend on those modules (i.e. you can require('sshpk') and it does not require any of those modules automatically -- you have to do something like make an ED25519 key and call .sign() on it before we require('tweetnacl')).

If webpack is not able to deal with the require() calls being optional and inside functions, I suggest that would be a bug in webpack, not in this module. But if there's somewhere where we are accidentally causing these modules to be require()'d when they aren't actually been used, I would like to know about that so I can fix it.

Sorry this has been sitting open for so long -- browser issues are not really a high priority for me (or the other people at Joyent who have worked on this module), and I can't really offer support for how to use sshpk with webpack in general (since I don't use it), but I'm happy to take reports of something more specific, like a require() happening when it shouldn't and get them fixed up.

@light24bulbs
Copy link
Contributor

@light24bulbs light24bulbs commented Oct 10, 2018

@arekinath Well, with webpack, even if they MIGHT get required in some function...they still need to be included. If the code ends up in the browser, and then the function gets called, the browser needs to have the package already loaded. It's just how webpack works, and for good reason. Webpack doesn't know and can't know if you're going to call that function or not. All it knows is that you might, so the dependency needs to be bundled. Does that make sense? It's not a bug in webpack. It absolutely needs to work that way, and can't work any other way.

At first glance, the way you are using optional dependencies makes sense. Unfortunately I think that is not the convention. Optional dependencies are almost always used for something that is more performant on a given system, but the module will run fine without them. They are definitely not intended for functionality that might not get called.

I think the very simple fix is simply to move them into dependencies. Almost everyone has been getting them anyway since optional dependencies are almost always installed. This is just breaking certain uncommon build systems that expect people to use the flag properly. Please head into the rush issue I referenced above for an example

@light24bulbs
Copy link
Contributor

@light24bulbs light24bulbs commented Oct 11, 2018

@arekinath arekinath changed the title fails to run when optional dependencies are not installed stop using optional deps to fix webpack Oct 11, 2018
joyent-automation pushed a commit that referenced this issue Oct 11, 2018
Reviewed by: Cody Peter Mello <cody.mello@joyent.com>
@arekinath
Copy link
Contributor

@arekinath arekinath commented Oct 12, 2018

Merged a change in the latest release based on @light24bulbs PR which moves all these into regular dependencies, and also changes the require() calls to just happen at startup (at the top of the files). Thanks!

@arekinath arekinath closed this Oct 12, 2018
This was referenced Oct 12, 2018
This was referenced Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.