Fix incorrect behavior on package rename

[savetheclocktower]

Suppose a user maintains a package called old-name and wants to rename it to new-name. When the user runs

ppm publish patch --rename new-name

ppm does these things (and other things, but these are the ones relevant to this discussion):

• automatically updates the name field in package.json to the new value (in fact, it will scold you if you try to do this manually); then
• asks the API whether the package already exists.
  • if so, it POSTs to /api/packages/old-name/versions/ with the package.json metadata;
  • if not, it POSTs to /api/packages/ with that same metadata (since this is a brand-new publish of a package).

If, in this scenario, the API got confused and told us that the package didn't exist, we'd end up treating this rename task as the publishing of a brand-new package. new-name would get published, but the API would not understand that it had anything to do with old-name.

As you might've guessed, that's exactly what is happening. It's happening because we end up asking the API if new-name already exists, not old-name! This guarantees that ppm publish [tag] --rename won't ever work.

The fix is easy — since we already keep track of the original name, we'll ask the API whether the original name exists.

I've tested this fix locally to ensure it hits the proper code path. In my test, the server responded with an error, but we can sort that out later.

---

I wrote new specs for this behavior. I also took this opportunity to clean up this repo's .eslintrc.js file; it referenced several plugins that weren't even installed.

[DeeDeeG]

Maybe the original intent was to simply check if the new name was available before trying to publish it?

I wonder if any of this is lost in translation from the CoffeeScript, and the callback-based era of the code in general (maybe lost in translation from decaf and/or the Promisification).

In either case, it's not immediatley intuitive what we need to do. But I think we most essentially need to:

• Make sure [new package name] isn't an existing package on the registry, ideally give a helpful error message if it is.
• Do whatever it is we do with the old package name (tell the backend the right incantation to make this occur)
  • Do we do anything with the old package name? For example, is the old name retired/redirected to the new one? Do we effectively unpublish/delete the old one... ?? ... Or does the old name simply keep existing like any other package?
• Publish package at [new name] if it's available.

I'm looking through the code now trying to puzzle it out.

[savetheclocktower]

Maybe the original intent was to simply check if the new name was available before trying to publish it?

No, because that code path is hit in all scenarios, including publishing a new version without renaming the package.

In either case, it's not immediatley intuitive what we need to do. But I think we most essentially need to:

• Make sure [new package name] isn't an existing package on the registry, ideally give a helpful error message if it is.

Luckily, the backend handles that part. When it sees that the package metadata has a name that doesn't match the name in the URL, it will check to see if the new name is available; if not, it'll return an HTTP error. That's the fix that we're making in this PR.

[savetheclocktower]

Do we do anything with the old package name? For example, is the old name retired/redirected to the new one? Do we effectively unpublish/delete the old one... ?? ... Or does the old name simply keep existing like any other package?

The old package name continues to exist in the names table on the package backend. It is treated as an alias of the new name and can't ever be used again. If you ask the package backend about package old-name, it will seamlessly return information about package new-name instead.

Edit: I'm using present tense here, but the status quo of ppm means that this behavior is broken. I'm describing what will happen once both this fix and the package-backend fix are landed.

[DeeDeeG]

Okay. Would love to see that backend fix land, and we can end-to-end test renames working before/rather than sort of blind-landing this one. The fix seems quite sensible from what you're saying. But when I test it, the end-user-apparent behavior is the same before/after the fix. (Assuming the end-user isn't console.log() debugging ppm or whipping out wireshark!) So, on some high level I trust this is the fix. And yet, there is an incomplete piece to this for me as a reviewer doing my duty.

I went ahead and looked at pulsar-edit/package-backend#271, reviewed what was said there and the code fix, left an Approve. I hope to see that one land, and we can confirm 100% that this one is working. If you don't mind that's the preferable order of things for me, especially if it looks like pulsar-edit/package-backend#271 is going to land anyway. Feel free to apprise me of any changes in proceedings or argue another case on how to see it. And thanks for the diligent issue-sleuthing and patching things up.

Some ppm workings are in much better shape due to stuff you've clearly/reproducible reported. And often committed fixes for! Thanks again.

[savetheclocktower]

If you don't mind that's the preferable order of things for me

Of course! Makes perfect sense.

@confused-Techie, I'd love to get this PPM fix into the next release, so if you can find the time to land package-backend#271 in the next few days, that'd give us plenty of time to verify this behavior.

[DeeDeeG]

I've been able to test and confirm this (alongside pulsar-edit/package-backend#271 now that it has landed and is live) does make the renaming process link the two package records in some way.

Particularly: Now that I renamed my test package [...]1823481346 to [...]1823481347, using ppm patched with this fix... loading the page for my "two packages" https://web.pulsar-edit.dev/packages/deedeeg-my-new-package-1823481346 and https://web.pulsar-edit.dev/packages/deedeeg-my-new-package-1823481347 now shows the details of the same package. They are linked on the backend, and the package at the new name takes over for(/is sent in response to) user requests for the old package (name).

Confusing terminology of how to describe this aside, and with certain details still up for discussion of how the best user experience and backend/frontend approach to handling this all is, this aligns with the previous expectation, which must have subtly broken at some point.

tl;dr: the fix works.

[DeeDeeG]

I am approving on the basis that the dual issue between the backend and ppm has been discussed enough that we have multiple people satisfied with this outcome, and on the basis that, in my testing, this works in alignment with the expectation established from that discussion. Which for the record, it has been argued simply follows from continuity with how apm and the original Atom package back end would have worked.

Without getting into the weeds of sleuthing through old code, it seems plausible this is more or less how it used to work, but perhaps more importantly, it's a way we're comfortable with it working now.

I implicitly trust that the new spec works, since the actual new src/ code does work in my testing.

Likewise, I don't really use eslint ever, but for those who do use it, and since we already had this file, we may as well update it for the current state of the repo -- I am once again implicitly trusting that the update to that file is fine.

Okay tl;dr Looks Good To Me, Approved.

(We can merge this and get the ppm updates into core. Since I've been directly involved with both PR's, I can be an easy approve for that, and we can try to get another approve on that if you want to. Either way.)

[DeeDeeG]

Merging, thanks for the fix!