Fix bugs found in `publish` after 1.112 release #116

savetheclocktower · 2023-12-16T22:58:54Z

These are just the ones I found when trying to run ppm publish patch, but I'm sure there are other bugs. The PPM rewrite was gigantic.

savetheclocktower · 2023-12-17T00:10:23Z

Added some tests. I don't love them — they construct a false reality! — but they're better than the nothing that we had.

asiloisad · 2023-12-17T00:13:13Z

I probably caught the same bug

savetheclocktower · 2023-12-17T01:27:57Z

These new tests pass locally for me. I'll get around to fixing them.

savetheclocktower · 2023-12-17T10:08:38Z

OK, found the problem with the tests. CI is good. This one is definitely a candidate for “squash and merge.”

Daeraxa · 2023-12-17T19:34:20Z

Do we need a 1.112.1 for this?

savetheclocktower · 2023-12-17T21:55:53Z

Yes. We should test other aspects of ppm to see if other bugfixes can be addressed in a patch release, but so far the only bugs reported are with publishing.

DeeDeeG

Putting all the whitespace comments I had in their own thread so it can easily be minimized when looking at the actual substantive feedback.

src/apm.js

src/publish.js

DeeDeeG

I was able to reduce the diff in src/ dir to the following and pass all the new tests:

diff --git a/src/publish.js b/src/publish.js
index 4dfef93..4571e44 100644
--- a/src/publish.js
+++ b/src/publish.js
@@ -169,7 +169,7 @@ have published it.\
       }
 
       const exists = await this.packageExists(pack.name);
-      if (exists) { return Promise.reject(); }
+      if (exists) { return false; }
 
       const repository = Packages.getRepository(pack);
 
@@ -324,7 +324,7 @@ have published it.\
 
     // Rename package if necessary
     async renamePackage(pack, name) {
-      if (name?.length <= 0) {
+      if ((name ?? '').length <= 0) {
         // Just fall through if the name is empty
         return; // error or return value?
       }

Along with updating one comment to reflect return false instead of Promise.reject() as a return value, is it okay to ask that the src/ dir changes of this PR be reduced to these changes? And I can volunteer to do this if the PR author is okay with my doing so and prefers not to spend the time fiddling with it themselves.

Open to feedback as to not reducing the src/ changes in the PR to the above, though.

DeeDeeG · 2023-12-18T02:21:43Z

spec/publish-spec.js

Thank you so much for adding tests, and sorry we didn't have adequate tests before!

DeeDeeG · 2023-12-18T02:27:47Z

src/publish.js

      return new Promise((resolve, _reject) => {
-        const requestTags = () => void request.get(requestSettings, (_error, response, tags) => {
-          tags ??= [];
-          if (response?.statusCode === 200) {
-            if (tags.find(elem => elem.name === tag) != null) {
-              resolve();
-              return;
+        const requestTags = () => {
+          request.get(
+            requestSettings,
+            (_error, response, tags) => {
+              tags ??= [];
+              if (response?.statusCode === 200) {
+                if (tags.some(t => t.name === tag)) {
+                  resolve();
+                  return;
+                }
+              }
+              if (--retryCount <= 0) {
+                return void resolve();
+              }
            }
-          }
-          if (--retryCount <= 0) {
-            return void resolve();
-          }
-
-          setTimeout(requestTags, interval);
-        });
+          )
+        };
+        setTimeout(requestTags, interval);
      });
    }


I tested with this part of the diff reverted, and tests pass. Looks like it does effectively the same thing as before this PR.

I dunno about this much diff to change indentation and tweak things a little. I would probably leave indentation the same, leave setTimeout() where it was unless there is a reason to move it (we're probably not hitting network errors, so this code path may not be being hit in CI or even in manual tests, admittedly?? But seems equivalent, regardless of which spot the setTimeout() is put in the loop, to my eyes.) And maybe just change tags.find() to tags.some() if wanting to slightly clean up that code to be slightly nicer-looking.

But I would just as much consider reverting all of this if it's not strictly needed to fix bugs.

I rewrote this section because the original code (in my opinion) went out of its way to be inscrutable.

An arrow function can skip surrounding braces if the entire body is a single statement. That doesn't mean it's a good idea to treat a complex multi-line statement as though it were a one-liner. I think that this is one of the reasons why the author ended up incorrectly putting the setTimeout on the inside of the requestTags callback instead of the outside.

I just realized you're right that I shouldn't have moved the setTimeout — I should've just invoked requestTags once outside the block. I'll make that change.

I'm still strongly in favor of reformatting that code; I'd prefer to do it now rather than in a separate PR.

And you're right that this code path still isn't hit in the tests — even in the new tests I wrote. I mocked anything that talked to the outside world. I don't know what else to do for a method whose only purpose is to wait until a remote server responds a certain way.

Re: tests, yeah, our tests are already not platinum-tier 110% complete, and I don't know off the top of my head how to mock an error code response from the hypothetical server. Certainly not going to ask for this, it's not obvious to me how doable it would be.

As for readability, yeah the original syntax was a little... weird. It's still kinda gnarly due to being so nested and having so many different JS features used in such a short span. I think you're right that a refactor is warranted for readability.

I ended up with this locally just so I could read and understand what was happening.

Code snippet (click to expand):

waitForTagToBeAvailable(pack, tag) { let retryCount = 5; const interval = 1000; const requestSettings = { url: `https://api.github.com/repos/${Packages.getRepository(pack)}/tags`, json: true }; return new Promise((resolve, _reject) => { const requestTags = () => { request.get(requestSettings, (_error, response, tags) => { tags ??= []; if (response?.statusCode === 200) { if (tags.some(t => t.name === tag)) { resolve(); return; } } if (--retryCount <= 0) { return void resolve(); } })}; setTimeout(requestTags, interval); }); }

Took me a lot of staring at it and having Pulsar highlight matching brackets/parentheses for me and trying to run it to see which line errored...

EDIT: My snippet even uses some unusual lack of indentation under the new Promise line, which might not help anyone but me, but this was the most sense I could make it make to my own eyes.

Alternate snippet with more standard indentation (click to expand):

return new Promise((resolve, _reject) => { const requestTags = () => { request.get(requestSettings, (_error, response, tags) => { tags ??= []; if (response?.statusCode === 200) { if (tags.some(t => t.name === tag)) { resolve(); return; } } if (--retryCount <= 0) { return void resolve(); } })}; setTimeout(requestTags, interval); });

src/apm.js

src/publish.js

savetheclocktower · 2023-12-18T05:30:48Z

src/publish.js

@@ -440,10 +445,11 @@ have published it.\
        if (rename?.length > 0) { originalName = pack.name; }

        let firstTimePublishing;
+        let tag;


Along with the two changes you mention in your review summary, this change is crucial. It moves tag to the outer scope so that it can be consumed later on.

savetheclocktower · 2023-12-18T05:33:57Z

src/publish.js

          setTimeout(requestTags, interval);
-        });
+        };
+        requestTags();


And this is the fourth change: invoking requestTags immediately after defining it. Without this, you can see that requestTags never gets called in the first place.

These four changes need to be part of any fix. I would also like this formatting change to make it in.

I understand what you're saying about the whitespace. If you're saying that all whitespace-related changes should go into their own commit, that's fine. If you're saying that they shouldn't make it into this PR at all, that'd be harder for me to agree with. But it's not something I'd go twelve rounds over.

DeeDeeG

Looks good to me.

(And thanks for bearing with the review.)

I am planning to manually rebase this branch so it's a cleaner, simpler series of commits, with any whitespace-only lines of diff in their own commit or commits as appropriate. (End result being the exact same code/diff you prepared, just a cleaner commit history.)

If you want to do the whitespace stuff or the rebase and cleanup, you can, but I was going to do it myself rather than make you worry about stuff outside what the code should look like.

Fix bugs found in `publish` after 1.112 release

DeeDeeG · 2023-12-18T07:14:02Z

Okay, I saved a copy of the original branch at https://github.com/DeeDeeG/ppm/tree/bug-fixes-original, for reference. (Tip of said branch being dfd1991).

Branch protection rules won't let me push master branch without it coming from a PR, so I'll update (force push) this PR with the tidied up commit history real quick...

DeeDeeG · 2023-12-18T07:16:37Z

Oh, actually... Hmm. I think I accidentally bypassed the branch protection rules (that I didn't recall we had set up) with admin powers.

DeeDeeG · 2023-12-18T07:17:32Z

Manually merged via commit f660a7a (a merge commit that I prepared on the CLI instead, using the usual text that would have been used if it were prepared via the github.com UI).

DeeDeeG · 2023-12-18T07:18:52Z

So, if you prefer the PR to have a purple icon and say "merged" I'll back out the manual merge and do it via the GUI here, but otherwise this'll be red and "closed", but the changes are in master branch at the moment one way or the other.

confused-Techie · 2023-12-18T07:20:12Z

@DeeDeeG For tracking and changelog reasons I'd say lets properly merge this one, but at the end of the day, as long as we remember the changes are in here, I suppose it doesn't matter too much

DeeDeeG · 2023-12-18T07:41:22Z

CI passed with my rebased commits (as they should, I didn't change any code at all), and I can confirm the "diff" between the original and the rebased version is none, only thing I did was the cleaner commit history.

Finally merging (via the github.com PR UI this time). Thanks much for the fix and added tests!

DeeDeeG reviewed Dec 18, 2023

View reviewed changes

src/apm.js Outdated Show resolved Hide resolved

src/publish.js Show resolved Hide resolved

src/publish.js Show resolved Hide resolved

src/publish.js Show resolved Hide resolved

src/publish.js Show resolved Hide resolved

src/publish.js Show resolved Hide resolved

DeeDeeG reviewed Dec 18, 2023

View reviewed changes

savetheclocktower commented Dec 18, 2023

View reviewed changes

DeeDeeG approved these changes Dec 18, 2023

View reviewed changes

savetheclocktower added 3 commits December 18, 2023 01:57

Fix bugs found in publish after 1.112 release

64da2fe

Add tests for publish

e5965a2

Fix whitespace

1ead662

DeeDeeG added a commit that referenced this pull request Dec 18, 2023

Merge pull request #116 from savetheclocktower/bug-fixes

f660a7a

Fix bugs found in `publish` after 1.112 release

DeeDeeG force-pushed the bug-fixes branch from dfd1991 to 1ead662 Compare December 18, 2023 07:24

DeeDeeG merged commit 7dfd9ca into pulsar-edit:master Dec 18, 2023
11 checks passed

DeeDeeG mentioned this pull request Dec 18, 2023

ppm: Update ppm to commit 7dfd9ca8cf877391fc6ef1d5 pulsar-edit/pulsar#842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugs found in `publish` after 1.112 release #116

Fix bugs found in `publish` after 1.112 release #116

savetheclocktower commented Dec 16, 2023

savetheclocktower commented Dec 17, 2023

asiloisad commented Dec 17, 2023 •

edited

Loading

savetheclocktower commented Dec 17, 2023

savetheclocktower commented Dec 17, 2023

Daeraxa commented Dec 17, 2023

savetheclocktower commented Dec 17, 2023

DeeDeeG left a comment

DeeDeeG left a comment •

edited

Loading

DeeDeeG Dec 18, 2023

DeeDeeG Dec 18, 2023 •

edited

Loading

savetheclocktower Dec 18, 2023 •

edited

Loading

savetheclocktower Dec 18, 2023

savetheclocktower Dec 18, 2023

DeeDeeG Dec 18, 2023 •

edited

Loading

savetheclocktower Dec 18, 2023

savetheclocktower Dec 18, 2023

DeeDeeG left a comment •

edited

Loading

DeeDeeG commented Dec 18, 2023 •

edited

Loading

DeeDeeG commented Dec 18, 2023

DeeDeeG commented Dec 18, 2023 •

edited

Loading

DeeDeeG commented Dec 18, 2023 •

edited

Loading

confused-Techie commented Dec 18, 2023

DeeDeeG commented Dec 18, 2023 •

edited

Loading

Fix bugs found in publish after 1.112 release #116

Fix bugs found in publish after 1.112 release #116

Conversation

savetheclocktower commented Dec 16, 2023

savetheclocktower commented Dec 17, 2023

asiloisad commented Dec 17, 2023 • edited Loading

savetheclocktower commented Dec 17, 2023

savetheclocktower commented Dec 17, 2023

Daeraxa commented Dec 17, 2023

savetheclocktower commented Dec 17, 2023

DeeDeeG left a comment

Choose a reason for hiding this comment

DeeDeeG left a comment • edited Loading

Choose a reason for hiding this comment

DeeDeeG Dec 18, 2023

Choose a reason for hiding this comment

DeeDeeG Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

savetheclocktower Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

savetheclocktower Dec 18, 2023

Choose a reason for hiding this comment

savetheclocktower Dec 18, 2023

Choose a reason for hiding this comment

DeeDeeG Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

savetheclocktower Dec 18, 2023

Choose a reason for hiding this comment

savetheclocktower Dec 18, 2023

Choose a reason for hiding this comment

DeeDeeG left a comment • edited Loading

Choose a reason for hiding this comment

DeeDeeG commented Dec 18, 2023 • edited Loading

DeeDeeG commented Dec 18, 2023

DeeDeeG commented Dec 18, 2023 • edited Loading

DeeDeeG commented Dec 18, 2023 • edited Loading

confused-Techie commented Dec 18, 2023

DeeDeeG commented Dec 18, 2023 • edited Loading

Fix bugs found in `publish` after 1.112 release #116

Fix bugs found in `publish` after 1.112 release #116

asiloisad commented Dec 17, 2023 •

edited

Loading

DeeDeeG left a comment •

edited

Loading

DeeDeeG Dec 18, 2023 •

edited

Loading

savetheclocktower Dec 18, 2023 •

edited

Loading

DeeDeeG Dec 18, 2023 •

edited

Loading

DeeDeeG left a comment •

edited

Loading

DeeDeeG commented Dec 18, 2023 •

edited

Loading

DeeDeeG commented Dec 18, 2023 •

edited

Loading

DeeDeeG commented Dec 18, 2023 •

edited

Loading

DeeDeeG commented Dec 18, 2023 •

edited

Loading