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

chore: simplify ca cert handling for app and inso #4738

Merged
merged 12 commits into from
May 11, 2022

Conversation

jackkav
Copy link
Contributor

@jackkav jackkav commented Apr 28, 2022

The first request could be getting blocked by file writes, this process is avoided by using the CAINFO_BLOB feature of node-libcurl

  • Moves ca cert loading outside of the request path to main app start and inso send-request.
  • Inso now uses os.tmpdir() rather than stubbing electron to point at os.tmpdir()
  • considering making all main/nodejs tmpdir calls use os.tmpdir() for consistency.
  • NOTE Oauth2 flow still blocking a clear renderer seperation, so we can't remove the getTempDir() helper just yet as react component code paths are still hit by oauth2 re-request.
  • blob support was added in feat: support curl_blob options JCMais/node-libcurl#300 looks like it should work 🚀 and allows us to eliminate two file writes!

Future work

  • remove electron shim from inso abstraction.
  • encourage oauth 2 re-request loop to respect architectural boundaries.
    • remove conditional window/electron and window/require hacks

Closes INS-1511

@jackkav jackkav changed the title fix: first request is slow chore: move another fs call to nodejs May 2, 2022
@jackkav jackkav requested a review from DMarby May 2, 2022 10:45
@jackkav jackkav changed the title chore: move another fs call to nodejs chore: simplify ca cert handling for app and inos May 10, 2022
@jackkav jackkav changed the title chore: simplify ca cert handling for app and inos chore: simplify ca cert handling for app and inso May 10, 2022
@jackkav jackkav marked this pull request as ready for review May 10, 2022 17:24
@jackkav jackkav requested a review from gatzjames May 10, 2022 17:38
Copy link
Contributor

@johnwchadwick johnwchadwick left a comment

Choose a reason for hiding this comment

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

LGTM, nice. I honestly forgot that we already got the node-libcurl side of things done :)

packages/insomnia/src/main.development.ts Outdated Show resolved Hide resolved
@jackkav jackkav enabled auto-merge (squash) May 11, 2022 15:40
@jackkav jackkav merged commit bddf13a into Kong:develop May 11, 2022
@jackkav jackkav deleted the fix/blocking-ca-write branch May 11, 2022 16:00
"build:app": "esr --cache ./scripts/build.ts --noErrorTruncation",
"build:main.min.js": "cross-env NODE_ENV=development esr esbuild.main.ts",
"build:sr": "npm run generate:ca-certs && esr esbuild.sr.ts",
"build:sr": " esr esbuild.sr.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jackkav sorry I didn't catch this in time, but maybe get rid of the extra space before esr in a future PR some time if you remember it.

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