fix(electron-updater): return correct release notes & name

[benjamincburns]

Refactors GitHubProvider.ts to fetch release information solely from the
GitHub API rather than the atom xml feed, as the atom xml feed doesn't
indicate whether or not a release is marked as prerelease, and because
the API was being used whenever allowPrerelease was false (likely the
vast majority of requests).

Makes it so that when the most recently published release is a
prerelease build, the GitHubProvider supplies the correct releaseName
and releaseNotes. Prior to this change, releaseName and releaseNotes
were always set to the name and notes from the latest published build,
even if it was prerelease, and even if allowPrerelease was false.

fixes #2742

[develar]

We cannot use API because it has rate limiting and updater will be blocked very soon.

[benjamincburns]

Per comments the original code already uses the API on nearly every update check. The rate limit is 60 calls per hour to the same URL (edit: see next comment). It should be simple to tell people to limit their update check timers to no faster than once per minute.

[benjamincburns]

@develar ah, I had it wrong in my previous comment. It's 60 requests per hour per IP, independent of API URL.

However, the fact still remains that the GitHubProvider does hit the API today (prior to my changes, that is) whenever updater.allowPrerelease is false. This is the default setting, and I think it's safe to say that most users leave this set to false, as prerelease builds might come with bugs.

Are you seeing complaints about the rate limiting impacting people's updates? If not, given how widely this module is used, I'd suggest that it's not an issue, and when it becomes one developers are working around it by reducing their request rates.

Finally, if you are still insistent that this PR can't be accepted as-is, please let me know what changes can be made to make it acceptable. I'm motivated to get this fixed quickly, and I'd rather not play a "will this make the maintainers happy?" guessing game.

[benjamincburns]

Just thought I'd add a bit more color to my thought process here.

I originally rewrote this to use the atom feed for 100% of the functionality. However I noticed early on that there's no data in the atom feed to determine whether the prerelease flag is set (see for yourself here). "No bother," I thought, and I added an isPrerelease method which tested semver.prerelease(releaseVersionString) != null (as semver.prerelease returns an array for prerelease builds).

All was well and good, until I ran yarn test. nsisUpdaterTest failed because @develar thoughtfully added a release to the test project which has a stable-looking version number, but actually has the prerelease flag set. I agree that this is a valid test, as not all projects use semver, and setting the prerelease flag is a common way to mask a build with potentially severe bugs from a marketing site's "download latest version" links (we've done exactly this with Ganache in the past).

This made me realize that I need to test directly whether or not the prerelease flag is set. My only options to do this are to go to the API, or to scrape the releases page directly. The latter option is easily ruled out as a bad idea, as it's likely to be brittle and equally likely to be against GitHub's ToS.

I then considered putting things back to the way they were, and only using the API to test whether a build is prerelease. However since the original code was already hitting the API for the majority of update checks (i.e., cases when allowPrerelease == false), and since the API provides very robust information about all releases via the repos/${owner}/${project}/releases endpoint in a single request, saving the extra request time by dropping the atom feed seemed like the logical thing to do.

As I said, I'm motivated to see this fixed/released, so you can expect a very fast turnaround on any suggested improvements. Until this is merged/released, I'm left to suffer with this nonsense in my package.json. :-(

[develar]

We do not use GitHub API in our electron-updater default provider. Only private. Please point me to code if you insist that it is used.

Are you seeing complaints about the rate limiting impacting people's updates?

Yes. That's why we don't use API :) You are from New Zealand, but think about people from third world countries like Russia and Belarus.

[develar]

I'm motivated to get this fixed quickly, and I'd rather not play a "will this make the maintainers happy?" guessing game.

I hate this game too. Don't worry — in most cases we accept PR as is (even with code style issues or so on, — this stupid work can be done by me).

I suggest — please add special option to electron-updater useGithubApi (to AppUpdater class) and introduce new GitHub Provider or modify existing one to respect this flag. So — by default useGithubApi will be false, maybe in the future we will enable it by default.

[benjamincburns]

We do not use GitHub API in our electron-updater default provider. Only private. Please point me to code if you insist that it is used.

@develar see getLatestVersionString in GitHubProvider: https://github.com/electron-userland/electron-builder/blob/master/packages/electron-updater/src/GitHubProvider.ts#L90-L104

This is called from getLatestVersion when this.updater.allowPrerelease == false: https://github.com/electron-userland/electron-builder/blob/master/packages/electron-updater/src/GitHubProvider.ts#L49

[develar]

@benjamincburns Thanks, it is bug, API should be used only for GitHub Enterprise.

[benjamincburns]

@develar ok, in that case how should prerelease status be evaluated? Is it fine to assume that projects are using semver?

[benjamincburns]

I ask because your existing tests guard against this assumption.

[develar]

Is it fine to assume that projects are using semver?

Definitely not.

There is one trick — latest release on GitHub will not report pre-release version (but it will be included into atom feed).

Hmm... so, I don't understand your issue — we do check right now that version specified in the atom.feed pre-release or not (using latest request). So, what's the issue?

[benjamincburns]

we do check right now that version specified in the atom.feed pre-release or not (using latest request)

/latest is an API endpoint. I thought you don't want to be using the API?

Either way, to understand the original issue I'm trying to solve, read through the current code (before my changes), and think about what happens when allowPrerelease is false, but latestRelease is a prerelease build. version gets set to the correct version string, but result.releaseName and result.releaseNotes get set to the releaseName and releaseNotes for the latest, prerelease build.

[benjamincburns]

that is, /latest isn't /latest, it's repos/${owner}/${project}/releases/latest

[benjamincburns]

Hopefully now you see my quandary, and why I wrote the code for this PR twice. :-) You either need to relax the constraint that the prerelease flag must be respected, or you need to relax the constraint that the API cannot be used.

[benjamincburns]

@develar actually, the easiest way to see why this is broken is to look here at the changes I had to make in the snapshot file here, and at Line 213. At present your snapshot doesn't match the state of the releases in your test project.

[benjamincburns]

That is, per https://github.com/develar/__test_nsis_release/releases.atom or https://api.github.com/repos/develar/__test_nsis_release/releases, there is no release with empty release notes, a releaseName of v1.5.2-beta.3, and a tag of v1.1.0.

[develar]

I have fixed API usage — 4f7f66e

[develar]

result.releaseName and result.releaseNotes get set to the releaseName and releaseNotes for the latest, prerelease build.

Got the issue. Indeed. Well, fix is easy — we must fix latestRelease. Feed XML contains information about several releases and we need simply filter out not suitable release notes (as getLatestVersion returns us latest stable release).

[benjamincburns]

Ok, re: 4f7f66e, I wasn't aware that this worked to produce a valid JSON response: curl -L -H "Accept: application/json" -i https://github.com/develar/__test_nsis_release/releases/latest. Is that documented anywhere? It seems likely that GitHub will break this if not.

[benjamincburns]

Well, fix is easy — we must fix latestRelease. Feed XML contains information about several releases and we need simply filter out not suitable release notes (as getLatestVersion returns us latest stable release).

@develar are you doing that now, or suggesting that I amend this PR to do this?

[develar]

Is that documented anywhere?

No.

It seems likely that GitHub will break this if not.

Tests will fail :) If you don't like it — just introduce option to use GitHub API as was proposed and use it in your project. But I doubt that it will be broken.

are you doing that now

No, I wait this fix from you, thanks in advance.

[benjamincburns]

If you don't like it — just introduce option to use GitHub API as was proposed and use it in your project.

I don't mind, but I'd rather you accepted #2750 in that case. If you review it I think you'll agree that it's a good change ;-)

No, I wait this fix from you, thanks in advance.

No problem, and thanks for sticking with me!

[benjamincburns]

@develar - changed as suggested. Hopefully this is more to your taste.

[benjamincburns]

whoops, looks like I had a couple of lint goofs - fixing those up now

[benjamincburns]

... that should do it

[benjamincburns]

@develar I'm under a tight deadline here - is there any chance that this will be merged/released in the next 24h? If not, I may wind up needing to publish a forked package under an obscure name to keep with my deadline. :-(

Why not use the scripts I linked to earlier? It turns out electron-builder doesn't like local filesystem dependencies, as it can't parse the file:// url as a version string.

[benjamincburns]

Actually, I think I need to go ahead and do that anyway so I can start testing my builds. If you're able to publish/release before my testing is done I'll happily unpublish my forked package.

[develar]

2.21.6 published

[benjamincburns]

Thanks! @develar!