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

fix: mac differential updater #8095

Merged
merged 21 commits into from Mar 4, 2024

Conversation

beyondkmp
Copy link
Contributor

@beyondkmp beyondkmp commented Mar 3, 2024

Fix copyfilesync error.

We should place copyFileSync inside the done function, ensuring that it is executed only after the file has been successfully downloaded and definitely exists within the corresponding folder

Copy link

changeset-bot bot commented Mar 3, 2024

🦋 Changeset detected

Latest commit: f52a95b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
electron-updater Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Mar 3, 2024

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit f52a95b
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/65e52dfb1774da000844e6be
😎 Deploy Preview https://deploy-preview-8095--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@beyondkmp
Copy link
Contributor Author

beyondkmp commented Mar 4, 2024

Currently, there are two issues, one is with canDownloadDifferentially, and the other is with the temp file.

  1. canDownloadDifferentially always returns false, which prevents full download.
  2. The destinationFile is a temp file, not the final file. If an error occurs, such as an incomplete download or removed, this temp file should not be used directly. It should only be copied after a successful download.
image

I have fixed these two issues. And it has been tested locally that both the initial download and the subsequent incremental downloads work well

@beyondkmp
Copy link
Contributor Author

@mmaietta Please help review again.

@mmaietta
Copy link
Collaborator

mmaietta commented Mar 4, 2024

LGTM, nice work!

@mmaietta mmaietta merged commit 53cec79 into electron-userland:master Mar 4, 2024
13 checks passed
@willemdjong
Copy link

FYI: The differential download on mac is quite slow compared to windows!

@mmaietta
Copy link
Collaborator

Slower than the full download on mac?

They're using the same differential implementation under-the-hood. My initial guess is that the diff for mac zips are larger than windows?

@beyondkmp
Copy link
Contributor Author

beyondkmp commented Apr 15, 2024

Currently, the incremental update on Mac has a flaw: the first update does not support incremental updates. It requires saving the update.zip locally first, and only supports incremental updates during the second update.

@willemdjong
Copy link

Much slower, only have small updates of 8GB or 13 GB...but compared to my full download of 250 GB it's like 5 times slower.

My initial guess is that the diff for mac zips are larger than windows? => this is exactly what I thought...

It requires saving the update.zip locally first, and only supports incremental updates during the second update. => discovered this already! But that's not a big deal for me.

@beyondkmp
Copy link
Contributor Author

Is your app size 250GB? It's very large. Incremental updates require time to calculate. Your package is quite large, which likely took a lot of time to determine which parts were incremental.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants