[pulsar-updater] Don't notify if Pulsar is running via yarn start
#679
Conversation
|
Belated approve+ from me. I'm not familiar with pulsar-updater's internals, but the changes look reasonable to me, and looks like it should do what the PR description says. Which is a good thing to do -- less notification spam! Thanks for this, this updater has seen a lot of feedback on the Discord and a lot of that feedback has been implemented. Thanks for the feature and continuing to refine it, very cool! |
|
Appreciate you @DeeDeeG |
| it("Returns spec mode if applicable", async () => { | ||
| it("Returns developer instance if applicable", async () => { | ||
| // We can't mock the atom api return from a package, | ||
| // So we will just know that if tests are running, it's in the Atom SpecMode |
There was a problem hiding this comment.
It's a little late, and I'm just now seeing this, but if it's important for this test to really check for "spec mode," that return value is now hidden behind the "dev mode" thing, due to the circumstantial fact that CI builds a version ending with -dev, and the -dev check presumably returns earlier than the "spec mode" check happens in the app code...
So IMO this test either needs the comment updated to explain that this is looking at whether the app has a -dev version string, or else the scenario is not useful at what the comment says it is checking and should be "fixed" to actually detect "dev mode" in our CI setup, or removed (but probably not removed, it's still checking something useful, IMO.)
Maybe off-topic aside note (click to expand):
(Aside: and I suppose there are a tiny few things elsewhere around the app that change with the version string ending in -dev, so you really can call it "dev mode". It's not strictly just that the version ends with -dev... So. That said, Regular releases can activate dev mode if they want, I dunno if I'm just off on an unrelated tangent here or what. Hm. Almost definitely overthinking it for this little comment that needs updating now that the meaning of the test has changed.)
There was a problem hiding this comment.
Really the important aspect was this test would check for however Pulsar runs while running tests.
Which prior to this addition of Developer Instance being added was spec mode since that's true, it just becomes also true that it's Developer Instance.
But as for you secondary tangent, don't worry, we also check for Developer Mode, which is when the editor is run in actual developer mode. I just thought Developer Instance could get the point across that it's a version of the editor literally being run via yarn start.
Also I will say, that talking further on this has made me realize a potential issue we may encounter in the near future.
With these changes as is, this test will fail when we do a Regular Release PR. Since at that point the version will no longer end in -dev.
So it may be worth it to consider checking for spec mode first, and changing the test back, or otherwise finding a new solution.
When a developer of Pulsar is running Pulsar locally, such as using
yarn startthe version of Pulsar would be reported as ending with-dev, which according to semver is a prerelease channel and would always be lower than the same version without the-dev.For example:
1.108.0.1.108.0-devpulsar-updatersees1.108.0-devand uses semver to compare it to the latest GitHub release of1.108.0pulsar-updaterprompts for an UpdateWhat this means, is that almost always when a developer is working on Pulsar they will be notified to update via
pulsar-updater.This PR includes a manual check within
pulsar-updaterto see ifatom.getVersion()ends with-devand if so will not emit a notification for an update.