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

deps: bump node-libcurl to latest prerelease version #2150

Closed
wants to merge 3 commits into from
Closed

deps: bump node-libcurl to latest prerelease version #2150

wants to merge 3 commits into from

Conversation

JCMais
Copy link
Contributor

@JCMais JCMais commented May 10, 2020

This is related to the following issue:
#997

And should unblock this:
#1847

Some code is based on the previous PR: #1528

Some notes:

Prebuilt binary

node-libcurl has been providing prebuilt binaries for Electron, however the current non-prerelease version, 2.1.1, does not provide binaries for Electron v3. I've added it back starting with 2.1.2-1.

Right now Insomnia is not using the prebuilt binaries, because the project uses electron-rebuild and it does not support node-pre-gyp, I've opened an issue there about adding support: electron/rebuild#367

If support is not added the prebuilt binaries can still be used by changing the bootstrap script on the root package.json to the following:

npm install && lerna bootstrap -- --runtime=electron --target=v3.1.13 --disturl=https://www.electronjs.org/headers && lerna run --parallel --stream bootstrap

And removing the electron-rebuild call from the insomnia-app bootstrap script.

This would simplify Insomnia build as there would be no need to build the native dependencies from source anymore. For the scripts used to build libcurl and their dependencies for node-libcurl itself, see this directory: https://github.com/JCMais/node-libcurl/tree/develop/scripts/ci

If Insomnia still needs to have control over the libcurl build, those environment variables:

NODELIBCURL_BUILD_STATIC: yes

NODELIBCURL_BUILD_STATIC: yes

Must be changed to npm_config_curl_static_build: true, which is the environment variable that would cause the following gyp variable to become true: https://github.com/JCMais/node-libcurl/blob/9aa421d2f39306a6e42afe84afe528164edd44c3/binding.gyp#L11

Curl.getVersionInfoString

Curl class now has a new static method getVersionInfoString, which returns more information than getVersion, it should be almost identical to the output of curl -V:

getVersion:

libcurl/7.69.1-DEV OpenSSL/1.1.1d zlib/1.2.11 WinIDN libssh2/1.9.0_DEV nghttp2/1.40.0

getVersionInfoString:

Version: libcurl/7.69.1-DEV OpenSSL/1.1.1d zlib/1.2.11 WinIDN libssh2/1.9.0_DEV nghttp2/1.40.0
Protocols: dict, file, ftp, ftps, gopher, http, https, imap, imaps, ldap, ldaps, pop3, pop3s, rtsp, scp, sftp, smb, smbs, smtp, smtps, telnet, tftp
Features: AsynchDNS, IDN, IPv6, Largefile, SSPI, Kerberos, SPNEGO, NTLM, SSL, libz, HTTP2, HTTPS-proxy

Just to not introduce that many changes, I've kept the current implementation of the wrapper to use getVersion.

Test actuallySend() › sends a generic request

This test was missing the HTTPAUTH option before, don't know why.

Uncommited files not ignored

After running the project those files were generated:

packages/openapi-2-kong/dist/index.js
packages/openapi-2-kong/dist/index.js.map

And they are not on .gitignore list, should they be added?

@netlify
Copy link

netlify bot commented May 10, 2020

Deploy preview for insomnia-storybook ready!

Built with commit 166c16f

https://deploy-preview-2150--insomnia-storybook.netlify.app

@gschier gschier self-requested a review May 12, 2020 21:44
@gschier gschier assigned gschier and unassigned gschier May 12, 2020
@gschier gschier self-assigned this May 14, 2020
@develohpanda
Copy link
Contributor

And they are not on .gitignore list, should they be added?

I have added them into .gitignore now 😄 #2165

@gschier gschier mentioned this pull request May 27, 2020
19 tasks
@gschier
Copy link
Contributor

gschier commented May 28, 2020

Thanks for kicking off this effort @JCMais! I added a bunch of stuff to this PR and merged it in #2223

Having prebuilt versions for Electron is a life saver 👍

@gschier gschier closed this May 28, 2020
@JCMais
Copy link
Contributor Author

JCMais commented May 28, 2020

@gschier glad it worked.

I will release a new version of node-libcurl sometime later this week with the changes made on the current pre-release version and a few other improvements.

@JCMais JCMais deleted the upgrade-node-libcurl branch June 1, 2020 21:47
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