Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: set default port for heroku:local to 5006 #2618

Merged
merged 20 commits into from
Feb 6, 2024
Merged

Conversation

k80bowman
Copy link
Contributor

@k80bowman k80bowman commented Feb 5, 2024

work item

This is a fix for #2515 that also does not re-introduce the defect found in #2457.

The approach to this fix is a bit unorthodox, so I am adding some notes here. It is not intended to be long-lived.

The Node Foreman package, which is the basis for our heroku:local commands, sets the default port to 5000. However, Apple decided to use that port for AirPlay, which led to two PRs (#2261 and #2475) that set the default port for the local commands to 5001 using oclif defaults. However, this caused the current bug (#2515). Setting the oclif default overrode any .env files being read through Node Foreman.

Additionally complicating this matter, Node Foreman has not been maintained in several years. An issue was filed about this default port problem, but we have no confidence that it will be resolved.

We plan in the near future to remove our dependence on Node Foreman. In order to fix this problem in the short-term, however, we have copied over the nf.js file from Node Foreman to our own repo and updated the default port to 5006. We are using 5006 instead of 5001 because it is not known to be used by other common software at this time.

Testing

  • Run yarn install and yarn build
  • Navigate to the /packages/cli directory
  • add a .env file that contains PORT=3000
  • Run ./bin/dev local
    • you should see the following:
[OKAY] Loaded ENV .env File as KEY=VALUE Format
11:20:53 AM web.1   |  it works (web)! port: 3000
11:20:53 AM worker.1 |  it works (worker)!
[DONE] Killing all processes with signal  SIGINT
11:20:53 AM web.1   Exited Successfully
11:20:53 AM worker.1 Exited Successfully
  • Delete PORT=3000 in the .env file inside /packages/cli
  • Run ./bin/dev local
    • you should see the following:
[OKAY] Loaded ENV .env File as KEY=VALUE Format
11:21:15 AM web.1   |  it works (web)! port: 5006
11:21:15 AM worker.1 |  it works (worker)!
[DONE] Killing all processes with signal  SIGINT

@k80bowman k80bowman requested a review from a team as a code owner February 5, 2024 16:55
Copy link
Contributor

@eablack eablack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request to add some comments, otherwise :shipit:

packages/cli/src/lib/local/run-foreman.js Outdated Show resolved Hide resolved
@k80bowman k80bowman changed the title fix: set default port for heroku:local to 5001 fix: set default port for heroku:local to 5006 Feb 6, 2024
Copy link
Contributor

@justinwilaby justinwilaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@k80bowman k80bowman merged commit 9687e82 into main Feb 6, 2024
8 checks passed
@k80bowman k80bowman deleted the k80/foreman-fix branch February 6, 2024 20:11
@jalada
Copy link

jalada commented Mar 1, 2024

A couple of members of the community gave feedback (me included) that a default port change probably warrants a major version bump, just chiming in to say it's a shame that this was a minor bump again. Please can listening port number changes be a major bump in future? It makes it much easier to sit up and take note when you're upgrading the CLI.

<3 you, Heroku!

@edmorley
Copy link
Member

I've just updated the default port listed on https://devcenter.heroku.com/articles/heroku-local from 5001 to 5006 to match this new default :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants