Skip to content

Make node-webcrypto-ossl an optional dependency.#689

Merged
amark merged 1 commit intoamark:masterfrom
71:patch-1
Jan 16, 2019
Merged

Make node-webcrypto-ossl an optional dependency.#689
amark merged 1 commit intoamark:masterfrom
71:patch-1

Conversation

@71
Copy link
Copy Markdown
Contributor

@71 71 commented Jan 16, 2019

node-webcrypto-ossl is a native Node package, and will only be used in a very specific instance (using SEA from Node).

In my case, and I suppose in many others', SEA in Node will never be used.

I'd like to have that dependency made optional, since a failure to require it is already handled. Furthermore on Windows it requires the entire native build toolchain to be installed, which is not ideal for such a small feature.

`node-webcrypto-ossl` is a native Node package, and will only be used in a very specific instance (using SEA from Node).

In my case, and I suppose in many others', SEA in Node will never be used.

I'd like to have that dependency made optional, since a failure to require it [is already handled](https://github.com/amark/gun/blob/535d6569fcff380faefe8ffafbeb658a1635b74f/sea.js#L183). Furthermore on Windows it requires the entire native build toolchain to be installed, which is not ideal for such a small feature.
@amark
Copy link
Copy Markdown
Owner

amark commented Jan 16, 2019

@71 I totally agree, but the community out-voted me on this :(

Although, I see it is this ?new? thing called optionalDependencies? Maybe that will let me get what I want + what the community wants?

Qs:

  1. Will this still auto-install on cloud peers, like Heroku, OpenShift, AWS, etc. in their automated build/packaging steps?

  2. Will this prevent install (or not fail?) on dev environments & small/limited devices, where it isn't needed / where developers want to "just try GUN out" and not have all the extra junk?

WELCOME TO THE COMMUNITY BTW!!!! :)

@71
Copy link
Copy Markdown
Contributor Author

71 commented Jan 16, 2019

@amark Hey, thanks for the quick reply!

Well, to cite the npm docs:

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object. This is a map of package name to version or url, just like the dependencies object. The difference is that build failures do not cause installation to fail.

Therefore, according to these docs:

  1. Yep, if it built before it'll keep building.
  2. Nope. If it failed before it will no longer fail, except at runtime (which is already handled by the aforementioned piece of code).

Additionally npm install accepts the --no-optional flag, which prevents optional dependencies from being installed.

@amark
Copy link
Copy Markdown
Owner

amark commented Jan 16, 2019

Okay, then I feel like this is safe enough that I should just do it :D :D :D thank you!!!

It would be nice if there was something like npm install gun defaults for developers the lightweight version (no optional dependencies), but on cloud environments if node_env = 'prod' or something that many of them already have flagged/set, then npm install gun is npm install gun --all or junk. Do you know something like this?

Will be pulling anyways.

How'd you find out about GUN? What are you planning on building?

@amark amark merged commit a07cfaa into amark:master Jan 16, 2019
@71
Copy link
Copy Markdown
Contributor Author

71 commented Jan 16, 2019

I thought about having two packages; one "core" package with only Gun, and another one with Gun + optional packages. Otherwise I haven't worked much with package.json so I can't help more than that, sorry!

I don't exactly remember how I found out about Gun, sorry (probably just a Google search with the right keywords: sync, javascript, database?).
I'm building a web app that can (optionally) be synced. I've tried three solutions thus far (Gun, RemoteStorage and Dexie), and it looks like Gun goes very well with what I want. I'll let you know if some things prevent me from using Gun further (just like node-webcrypto-ossl almost did!).

@finwo
Copy link
Copy Markdown
Contributor

finwo commented Jan 16, 2019

@amark Did you publish this yet? It would save us a lot of time installing dependencies (not using it either)

@amark
Copy link
Copy Markdown
Owner

amark commented Jan 16, 2019

@71 yaaaay that you are liking GUN better so far :) thank you for reporting & contributing to this issue rather than just walking away, very much appreciate that.

@finwo you should have spoken up and defended me then when the community did a PR to switch these deps from devDeps to regular deps! :P :P I didn't want to do it but everybody else in the issue was shaming me for not having them hard-coded. (not that I fall to peer-pressure, but on things like this, I certainly want to be mindful of community).

This was opened just a little bit after ...8 was published, so this won't go out till ...9 which hopefully will be by end of week. Unless

@71 & @finwo you have critical needs as an excuse for me to publish sooner :P , sounded like @finwo you hinted on chat you guys actually running this in production !? I'd love to hear more about the use case :D :D :D !

@finwo
Copy link
Copy Markdown
Contributor

finwo commented Jan 16, 2019

@amark I wasn't monitoring the discussions back then. The release-cycle is up to you. Both larger and micro-updates have pros and cons. I would even separate SEA into a separate package, to keep logical layers separated.

The platform I'm using it for is planned to go into production at the end of this month, but I won't pollute this PR with further details.

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

Successfully merging this pull request may close these issues.

3 participants