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

Upgrade node-libcurl #2223

Merged
merged 37 commits into from
May 28, 2020
Merged

Upgrade node-libcurl #2223

merged 37 commits into from
May 28, 2020

Conversation

gschier
Copy link
Contributor

@gschier gschier commented May 27, 2020

This PR builds on top of #2150

Test build located here: https://github.com/Kong/insomnia/releases/tag/designer%402020.1.4-alpha.1

Summary

This PR makes the following changes:

  • Update node-libcurl to latest release to prepare for Electron upgrade [Build] Update Electron #1847
  • Switch to using node-libcurl static prebuilt binaries so we no longer have to build
    • Move TravisCI Mac builds back to GitHub Actions
    • Remove Docker scripts for Linux builds

To-Do

  • Update CI release logic
    • Add NPM_CONFIG_... env vars to CI because .npmrc was being ignored in CI
  • Now rely on new node-libcurl prebuilt binaries instead of building ourselves
    • Remove Docker from build pipeline (now build directly on GH Actions)
    • Remove electron-rebuild dependency
    • Ditch TravisCI and go back to GH Actions for Mac builds (were on TravisCI before because they had the oldest version of macOS, which was required to get the most compatibility for linking to the OS version of libcurl – no longer necessary because we use a static version)
    • Migrate secrets from TravisCI to GH Actions
  • Add .npmrc to project to tell NPM to install for Electron target instead of host OS
  • QA on macOS 14 and 15, Ubuntu 18, Windows 10
    • Timeline tab
    • Curl-specific auth formats
    • Redirects
    • SSL
    • Client certificates
    • Unix sockets
    • file:// URIs
  • [MISC] replace font-scanner with font-manager (font-scanner was an unmaintained fork)
  • Run unit tests
  • Verify reasonable binary sizes ~80MB

@netlify
Copy link

netlify bot commented May 27, 2020

Deploy preview for insomnia-storybook ready!

Built with commit d7f10ef

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

@netlify
Copy link

netlify bot commented May 27, 2020

Deploy preview for insomnia-storybook ready!

Built with commit 24b6881

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

@gschier gschier force-pushed the deps/upgrade-node-libcurl branch from 2f6ab7c to 9737076 Compare May 27, 2020 19:56
@gschier gschier marked this pull request as ready for review May 27, 2020 21:30
@gschier gschier requested a review from develohpanda May 27, 2020 21:31
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

^Enough said? 😄

LGTM, a couple of questions but not major.

Do you know if the requirement for Windows Build Tools was to allow libcurl to be built? This change might make it easier to dev on Windows too!

.github/workflows/release-designer.yml Show resolved Hide resolved
.github/workflows/release-core.yml Show resolved Hide resolved
packages/insomnia-app/app/network/network.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/node-libcurl/curl.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/node-libcurl/curl.js Outdated Show resolved Hide resolved
@@ -74,7 +73,7 @@ module.exports.start = async function(forcedVersion = null) {
await copyFiles('../app/static', '../build/static');
await copyFiles(`../app/icons/${appConfig().appId}`, '../build/');

// Generate package.json
// Generate necessary files needed by `electron-builder`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

packages/insomnia-app/scripts/build.js Show resolved Hide resolved
// Explicitly set userData folder from config because it's sketchy to
// rely on electron-builder to use productName, which could be changed
// by accident.
if (!isDevelopment()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the product name in one of these commits without realizing electron-builder used it as the userData folder name. This adds a new option in the config and sets the userData folder explicitly so it's less likely we'll screw it up later.

@gschier gschier requested a review from develohpanda May 28, 2020 00:51
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.

4 participants