@deansheather filed a bug related to the Windows Firewall prompt coming up with every new binary (i.e. every time we downloadcoder to an updated version).

After some initial testing, it appears keeping the binary in the same place will make the prompt only appear on the first time.

Previous implementation kept the version in the path.

We decided to refactor things to add support forETag on the server and then use those changes on the client (aka in the extension).

This means now the client will tell the server "Here's the version I have. What should I do?" and send the appropriate headers.

Checklist

if needed, change path/name of location to not include version
if the binary exists, SHA1 it and use it as theIf-None-Match header in your request
Server Response changes
- on 200 OK: move the binary on top of the old one
- on 304 Not Modified: delete the temp file and use the old binary

Testing Plan

TBD

Fixes#23

jsjoeio changed the title~~wip~~fix: keep binary in same place, update if needed

Jan 17, 2023

jsjoeio force-pushed thejsjoeio/fix-binary-windows branch from4caf765 tod06c873Compare

January 17, 2023 21:04

jsjoeio commented

Jan 17, 2023

View reviewed changes

src/storage.tsShow resolvedHide resolved

src/storage.ts OutdatedShow resolvedHide resolved

src/storage.tsShow resolvedHide resolved

src/storage.ts OutdatedShow resolvedHide resolved

jsjoeio requested a review fromkylecarbs

January 17, 2023 21:07

jsjoeio self-assigned this

Jan 17, 2023

jsjoeio requested a review fromdeansheather

January 17, 2023 21:07

Copy link

ContributorAuthor

jsjoeio commentedJan 17, 2023

What's the easiest way to test these changes locally@kylecarbs ?

deansheather approved these changes

Jan 17, 2023

View reviewed changes

src/storage.ts OutdatedShow resolvedHide resolved

deansheather previously requested changes

Jan 17, 2023

View reviewed changes

Copy link

Member

deansheather left a comment

Choose a reason for hiding this comment

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

oops meant to request changes

jsjoeio requested a review fromdeansheather

January 17, 2023 22:19

jsjoeio marked this pull request as ready for review

January 18, 2023 15:49

deansheather reviewed

Jan 18, 2023

View reviewed changes

Copy link

Member

deansheather left a comment

Choose a reason for hiding this comment

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

the logic seems OK but I haven't tested it

src/storage.tsShow resolvedHide resolved

Copy link

ContributorAuthor

jsjoeio commentedJan 18, 2023

@deansheather re: testing

Let me make these changes, build to avsix then send to you via Slack to test

Copy link

ContributorAuthor

jsjoeio commentedJan 18, 2023

I tested locally on macOS and something is off

Seems like it's trying to use the binary before it's done renaming it?

jsjoeio added4 commits

January 18, 2023 09:32

refactor: remove version from binaryPath

82717c2

This removes the version from the binary path to ensure that the pathalways remains the same on OS's.This way, tools like Windows Firewall and macOS Firewill will onlyprompt for a security check on the first install but not subsequentinstalls.

refactor: update logic for downloading binary

197385a

This makes significant changes to the `fetchBinary` logic.First, it refactors a couple pieces of logic into methods on the`Storage` class to make the code more readable.Then it modifies the flow to first check if the binary is outdated. Ifit is, then it downloads the latest version.

refactor: only make request once

400ba7a

refactor: switch on status to avoid piping

369a823

jsjoeio force-pushed thejsjoeio/fix-binary-windows branch from36da848 to369a823Compare

January 18, 2023 16:32

Copy link

ContributorAuthor

jsjoeio commentedJan 18, 2023•
edited
Loading

I'm going to try rebasing onmain and then rebuilding and testing again.

EDIT: same issue

Copy link

ContributorAuthor

jsjoeio commentedJan 18, 2023

I figured it out - I was trying torm the file aftermving and so it was failing because it couldn't remove a file that doesn't exist 🤦🏼‍♂️

jsjoeio added2 commits

January 18, 2023 10:42

refactor: delete rm(tempFile)

923225b

chore: add package:prerelease script

0a0b041

This will build a `.vsix` file locally marked as a pre-release which ishandy for testing.

Copy link

ContributorAuthor

jsjoeio commentedJan 18, 2023

Tested locally and seems to work 👍🏼

jsjoeio requested a review fromdeansheather

January 18, 2023 18:02

jsjoeio added3 commits

January 19, 2023 11:05

feat: mv binPath to old before downloading new

eafac66

This makes a couple new changes:- if 200, move old binPath to binPath.old-- try removing .old binary- on start, cleanUpOldBinaries

refactor: move and catch things

6f6adb7

- catch all potential errors from `fs`- only rename binPath to old right before replacing with tempFile

refactor: generate Etag correctly, add to request

7a31663

This generates the Etag correctly using a readableStream and the appendsit to the headers. This also adds some logging around etag, binPath andstatus code.

jsjoeio dismisseddeansheather’sstale review

January 20, 2023 17:19

outadated

Copy link

ContributorAuthor

jsjoeio commentedJan 20, 2023

@deansheather ready for a new review!

deansheather approved these changes

Jan 20, 2023

View reviewed changes

Copy link

Member

deansheather left a comment

Choose a reason for hiding this comment

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

Looks good and works on Windows for me. The logic seems OK but I'm not a big typescript guy so someone else should approve too

src/storage.ts OutdatedShow resolvedHide resolved

src/storage.tsShow resolvedHide resolved

jsjoeioand others added2 commits

January 20, 2023 11:01

Update src/storage.ts

5528dc8

Co-authored-by: Dean Sheather <dean@deansheather.com>

Update src/storage.ts

db53aaa

Co-authored-by: Dean Sheather <dean@deansheather.com>

Kira-Pilot reviewed

Jan 20, 2023

View reviewed changes

package.jsonShow resolvedHide resolved

Kira-Pilot reviewed

Jan 20, 2023

View reviewed changes

src/storage.tsShow resolvedHide resolved

Kira-Pilot approved these changes

Jan 20, 2023

View reviewed changes

Copy link

Member

Kira-Pilot left a comment

Choose a reason for hiding this comment

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

Really appreciate the comments you left!

jsjoeio merged commitec4aa92 intomain

Jan 20, 2023

jsjoeio deleted the jsjoeio/fix-binary-windows branch

January 20, 2023 22:44

Labels

None yet

3 participants

Movatterモバイル変換

fix: keep binary in same place, update if needed#33

fix: keep binary in same place, update if needed#33

Uh oh!

Conversation

jsjoeio commentedJan 17, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Description

Checklist

Testing Plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsjoeio commentedJan 17, 2023

Uh oh!

Uh oh!

deansheather left a comment

Choose a reason for hiding this comment

Uh oh!

deansheather left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsjoeio commentedJan 18, 2023

Uh oh!

jsjoeio commentedJan 18, 2023

Uh oh!

jsjoeio commentedJan 18, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

jsjoeio commentedJan 18, 2023

Uh oh!

jsjoeio commentedJan 18, 2023

Uh oh!

jsjoeio commentedJan 20, 2023

Uh oh!

deansheather left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kira-Pilot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsjoeio commentedJan 17, 2023•
edited
Loading

jsjoeio commentedJan 18, 2023•
edited
Loading