fix: Pin express to 5.0.0-alpha.8

[edvincent]

Fixes #4900

Since [email protected] was released on 2022-02-14, any new install was bumped to use that version. That's because the way semver's caret works, and how beta > alpha. And it just so happens that there's a breaking change in there apparently.

When developing, the yarn.lock file is used, which itself was pinning to [email protected] which led to everything working well.

But when a package gets published, the lockfile gets ignored by yarn publish (see the missing yarn.lock in https://unpkg.com/browse/[email protected]/). That being said, even if present, it would still be irrelevant, because when installing code-server globally, it's still considered of a dependency of a global package. And the lockfiles from dependencies also get ignored when installing them. (See this Libraries vs Applications section for the reasons why).

---

I ran the release manually, and then installed the release.tar.gz package - and it all worked as expected 🎉

[edvincent]

Thoughts on extending the CI to not only run the end-to-end tests on the binary, but ### also on the npm version? Happy to give it a go, but it might extend the duration of the CI?

And I would mean doing it from installing the NPM package from release.tar.gz rather than the instance exposed by yarn watch - which I believe is what @code-asher was reporting in this commit?

[jsjoeio]

Thank you so much for going the extra mile and explaining why we need this!

[jsjoeio]

But when a package gets published, the lockfile gets ignored by yarn publish (see the missing yarn.lock in https://unpkg.com/browse/[email protected]/).

TIL! I don't think myself or @code-asher realized this.

That's because the way semver's caret works, and how beta > alpha.

I guess that bit us in the butt here. Well, won't make that mistake again. Thank you for all the links as well.

[jsjoeio]

Thoughts on extending the CI to not only run the end-to-end tests on the binary, but ### also on the npm version? Happy to give it a go, but it might extend the duration of the CI?

And I would mean doing it from installing the NPM package from release.tar.gz rather than the instance exposed by yarn watch - which I believe is what @code-asher was reporting in this commit?

I guess that would have prevented this bug, right? Hmm...I mean I'm open to it since it would hopefully mean less bugs. Thoughts @code-asher?

[jsjoeio]

The change itself looks good but moving to "Request changes" so we know we need to update the yarn.lock file for this to be ready to go 👍

[code-asher]

Makes sense to me, we could run tests on the NPM artifact before publishing it perhaps.

[edvincent]

The change itself looks good but moving to "Request changes" so we know we need to update the yarn.lock file for this to be ready to go 👍

Ah damn - the automation didn't run for me (as I was a first-time contributor), and I guess something is different on the way it builds it. Let me see if I can try to amend the PR for that too.

I guess that would have prevented this bug, right? Hmm...I mean I'm open to it since it would hopefully mean less bugs. Thoughts @code-asher?

Yes, it would have caught it :) Let me give it a go (always been curious how this Github Actions/Worfklows work, sounds like a perfect use-case to learn), but I'll track that in a different PR/Issue to not block this here.

Makes sense to me, we could run tests on the NPM artifact before publishing it perhaps.

You mean before publishing the NPM artifact, or before publishing any new version? As in, likely you want to prevent the binaries to be released too if the artifacts don't work? To avoid inconsistencies?

Also - rather than at publishing time, wouldn't a check that always runs (even when not publishing) would help catching the issue as early as possible? Rather than realizing at publishing and struggling to pin-point when was the regression introduced? Not sure when they run today.

[code-asher]

I meant just before publishing the NPM artifact since the standalone releases use --frozen-lockfile so the dependencies will be the same there whether we test them at commit time or release time.

But yeah maybe testing earlier would be better. Maybe we can just remove --frozen-lockfile and that will be enough? Or perhaps test once with --frozen-lockfile and once without? I wish the lock file was used for end users...we essentially have two fundamentally different versions of a release because of this.

[code-asher]

Actually we could just pin all the versions without using ^ and friends right (do what was done here to all the dependencies)? Not sure how much harder that would make updates though.

[edvincent]

I wish the lock file was used for end users...we essentially have two fundamentally different versions of a release because of this.

Yes, that's also the only thing I kept thinking after digging the reasons why.

Actually we could just pin all the versions without using ^ and friends right? Not sure how much harder that would make updates though.

Well, that would just mean that rather than updating the lockfile, you'd only need to update the package.json...

The main problem/side-effect would likely be the release size. Pinning all the versions wouldn't allow the dependencies to be folded down based on the best version for you OR your dependencies... Basically anywhere where "deduped" is written in https://editor.mergely.com/WfQ5xHTn/ might be a brand new package to bring (and its own dependencies, too), as there no guarantee that the version you want to bring would match your dependencies' criteria.

Let me dig around and see if there's a better way to fix the problem across the package.

[codecov]

Codecov Report

Merging #4918 (8fa1fd6) into main (b3cf4c3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4918   +/-   ##
=======================================
  Coverage   69.96%   69.96%           
=======================================
  Files          29       29           
  Lines        1655     1655           
  Branches      364      364           
=======================================
  Hits         1158     1158           
  Misses        423      423           
  Partials       74       74           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3cf4c3...8fa1fd6. Read the comment docs.

[jsjoeio]

[jsjoeio]

I'm going to update your branch then I can merge! @edvincent really means a lot that you reported this bug, dug in and opened a PR - very rare to see that happen. Thanks so much!

[edvincent]

FWIW - Downloaded the artifact from the CI run (https://github.com/coder/code-server/actions/runs/1913341110) and installed it locally. All working as expected 🎉

ubuntu@ip-172-26-6-3:~$ grep "express" package-lock.json
        "express": "5.0.0-alpha.8",
    "express": {
      "resolved": "https://registry.npmjs.org/express/-/express-5.0.0-alpha.8.tgz",

I'm going to update your branch then I can merge! @edvincent really means a lot that you reported this bug, dug in and opened a PR - very rare to see that happen. Thanks so much!

My pleasure. As I was saying, happy to give back to the project, been really helpful to have this and really enjoy the in-browser experience - so fixing problems is the least I could do.

(And selfishly, been learning a lot of stuff too, so enjoyed contributing.)