Closed confused-Techie closed 11 months ago
If you wanna address some of the feedback, then I can wait, otherwise, I am inclined to merge.
(I'm feeling a little move-fast about this PR today myself, but any efforts to arrive at even better approach is appreciated and I'd wait however long it'd take for it if you were so inclined to find improvements along those lines as I mentioned. But they are optional, since this is kind of a hack and workaround anyway. The only alternative approaches we might find could still end up being various degrees of hacks/workarounds.)
And once I review whatever all else has been committed since ppm
was synced to core repo, I (or someone else if I can't today) can do a ppm
bump in core repo.
@DeeDeeG So on what you've mentioned here.
My other PR over in pulsar
doesn't address what you mentioned.
The difficult part about just not asking ppm to check for updates of certain packages, is that settings-view
actually doesn't ask for updates to any specific packages.
Instead settings-view
just runs the generic command to ask PPM to check for updates for all installed packages. Which would include our bundled ones. So really this may be the only way to stop this behavior from occurring.
As for adding a test, I'd be more than happy to take a look to see if we can do that. But I'd like to say, I don't see any way we can easily revert this PR later down the road without significant changes.
The difficult part about just not asking ppm to check for updates of certain packages, is that
settings-view
actually doesn't ask for updates to any specific packages.
@confused-Techie so there's no obvious way to check the repository of "all packages" in this instance and not ask for the update?
Is it really asking ppm to "go check all the packages at once to see if upgrades are needed?" Oh, wait... Yes it is.
% ppm upgrade --help
Usage: ppm upgrade
ppm upgrade --list
ppm upgrade [<package_name>...]
Upgrade out of date packages installed to ~/.pulsar/packages
This command lists the out of date packages and then prompts to install
available updates.
[ . . . snipped . . . ]
Okay, I see why it wouldn't be easy to do this part over at core repo, and why we have to fix it in two different places, since we're fetching updates for packages for two separate purposes, in two separate ways across settings-view
package.
Thanks for the clarification nudging me to check closer what command this PR is editing, I understand better now.
@DeeDeeG Exactly, which is unfortunate. But yeah settings-view
doesn't even know what packages are being checked. So it'd be a significant code change to even make it aware of the packages to them go ahead and not check them. But hopefully this fix isn't too hacky to remain in the codebase for a bit of time.
If we really worried about it's implementation, we could add a flag to the command, such as --skip-core
which would optionally enable this feature, so that it worked here, but otherwise wouldn't provide any strange behavior for end users
Lets merge, woot!
On closer look, this ppm upgrade
command is already checking only packages under ~/.pulsar/packages
. So, bundled packages should be skipped right out of the gate, so to speak. (And it seems to be already not hitting the backend API for packages installed via ppm link
or installed from git URLs either, at least in testing on my machine.)
So, even without this PR, ppm upgrade
should already be ignoring any truly bundled-with-the-editor packages already, and most (all?) ways folks would get a package with repository matching our core repo URL.
Furthermore, with familiarity of how the packages' package.json
s are formatted on-disk in Pulsar, this seems to be another case where packages get the object
form of repository
, where repository.url
is the real URL we are looking for, and it ends in .git
.
For example, when I run ./bin/ppm upgrade --verbose
, it is only trying to check for upgrades to my two non-bundled, non-ppm link
ed, installed packages from the Pulsar package registry.
With some console.log()
peppered into the code:
% ./bin/ppm outdated
NOTTTT skippinggggg
pack.repository is:
{ type: 'git', url: 'git+https://github.com/dracula/atom.git' }
NOTTTT skippinggggg
pack.repository is:
{ type: 'git', url: 'git+https://github.com/dracula/atom-ui.git' }
Package Updates Available (0)
└── (empty)
(And it does have some support for checking for updates for packages from git URLs, but these are handled with git
commands and fetching from the packages' respective repos, not hitting the package backend API server.)
I dunno if we want to revert the change, considering this is such a small and not-hurting-anything change, but it also isn't doing the thing it's meant to be doing in real-world testing, I think. (Given some further research into things since this was merged.)
Sorry to be a bother about it, I just noticed and wanted to share what I found.
@DeeDeeG Great catch here, and now that I think about it, you are totally right. This wouldn't ever see any packages that are bundled.
So yeah this is kinda pointless, and while harmless, I'd hate for it to add uncertainty into how PPM behaves to someone reading it in the future. So seems best to revert this PR.
There's a shiny "Revert" button next to the "merge" event if scrolling up the history of this PR.
I'm somewhat curious to try it. Hmmmmmmmmmmmm. I might try that in a bit if no objections, or if you want to try it, go for it.
EDIT: Ah. When hovered, it says it will "Create a new pull request to revert these changes". So, nothing too crazy. Makes sense.
@DeeDeeG I've never seen this option. So that sounds like a plan then, lets give it a wirl
This PR allows PPM to skip checking for updates for bundled packages within Pulsar.
This only works if the package is not a valid git repository, which is true for the vast majority of our bundled packages.
Although, some outliers to this exist, those can be determined by checking the remaining items here. As for all other of the 81 bundled packages, this should allow them to be silently skipped. Assumed to always be up to date by PPM.
This is done by checking the
repository
of thepackage.json
of the package and determining if it's the same as the repository of Pulsar itself.