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

Changing the DEV_PORT doesn't update the watcher reload_port shown in terminal #1874

Closed
jwoertink opened this issue Apr 23, 2024 · 12 comments
Closed
Labels

Comments

@jwoertink
Copy link
Member

From this comment: luckyframework/website#1180 (comment)

Basically you change DEV_PORT env to say 3002, but the output message still shows 3000

🎉 App running at http://127.0.0.1:3000
@jwoertink
Copy link
Member Author

I just tried to recreate this, and it all seems to be working as expected...

DEV_PORT=3002 lucky dev

image

I'm guessing where the confusion may have been is if you add your DEV_PORT to your .env, it won't update. This is because it doesn't load the .env until it's already booted your app (i.e. at runtime). I think this is just a matter of bad documentation.

@zw963
Copy link

zw963 commented Sep 16, 2024

I'm guessing where the confusion may have been is if you add your DEV_PORT to your .env, it won't update. This is because it doesn't load the .env until it's already booted your app (i.e. at runtime).

Should we consider fixing it?

i open a older lucky project, when i start it use lucky dev, it told me, web server listen on 3000, but, try a lot times, no work.
until i check all my port in my laptop, found the port 3002 is listen when server was start.

yes, there is a .env, which content is: DEV_PORT: 3002, it spent me some time to discover it.

Can we delay the output of App running at http://127.0.0.1:3000 until it the dotenv loaded to always give the correct result?

Thanks

@jwoertink
Copy link
Member Author

Should we consider fixing it?

I don't know if we can fix it because these values can't be set at runtime. They have to be set before compile-time. I did notice I forgot to update the docs on this, so I added luckyframework/website#1383

@zw963
Copy link

zw963 commented Sep 17, 2024

I don't know if we can fix it because these values can't be set at runtime. They have to be set before compile-time

Hi, what i thought is, this has nothing to do with when the value was set, it relates to when the port info is shown to the user. can we delay displaying this message? until runtime know the correct port?

@jwoertink
Copy link
Member Author

can we delay displaying this message?

We might be able to, but it might require a lot of refactoring. I don't have any time to look in to it though. If you would like to open a PR and take a shot, please do 👍

Basically you would have to figure out how to delay building the watch task until after your app is built.

lucky/tasks/watch.cr

Lines 220 to 222 in dd21f8b

private def running_at_message
" 🎉 App running at #{running_at} "
end

@zw963
Copy link

zw963 commented Sep 18, 2024

If you would like to open a PR and take a shot, please do 👍

I will.

lucky/tasks/watch.cr

Lines 220 to 222 in dd21f8b

private def running_at_message
" 🎉 App running at #{running_at} "
end

Wired, i change the return string of running_at_message method in the project lib/lucky/tasks/watch.cr folder, then run lucky dev again, there output is not any changes at all. is there a way to make lucky dev to rebuild use the changed code in the lib folder?

Thanks.


EDIT: never mind, i deleted the build file in the bin/, it works now.

I found this issue caused by the prebuilt task bin/lucky.watch which postinstall when install lucky shards, if delete that file, lucky watch will compile before use it, and always can return expected result. any idea?

@zw963
Copy link

zw963 commented Sep 18, 2024

Compared to a bit speed improvement when run lucky dev which use prebuilt task bin/lucky.watch, i thought the behavior correct is more important.

I tought two solutions:

  1. Don't print running port in the tasks/watch.cr, instead, move those logic out of watch, e.g. add it into tasks folder in the generated project?

  2. comment this line in shards.

both solution are easy, perhaps you have a better way?

@jwoertink
Copy link
Member Author

  1. I'm not sure I understand. You want the watch to build against some local URI, but print in another code somewhere else? I don't think that would actually fix it, and it seems like it break apart the role of the watch task.

  2. My app takes 2 minutes to build in development, and commenting out that task makes it take longer. I don't think that is a good solution until the Crystal compiler is faster.

I know those are easy ways, but I don't believe either of them are the correct way. I did look in to this, and I don't see any clear paths for fixing this, but I also find it a very small use case since you don't ever need to use DEV_PORT if you're using Nox as your process runner. It's mainly just provided as an escape hatch, and adding DEV_PORT=3002 lucky watch to your Procfile.dev should be more than sufficient without having to break any code. In this case, it just becomes better documentation luckyframework/website#1383

@zw963
Copy link

zw963 commented Sep 19, 2024

I'm not sure I understand. You want the watch to build against some local URI, but print in another code somewhere else? I don't think that would actually fix it, and it seems like it break apart the role of the watch task.

What i means is, watch task still playing it role, but if it can't print the port correctly, it shouldn't print it to mislead users.
this is a serious bug, instead, add following code into any place in the generated project is better. e.g. add following code into the end of src/start_server.cr before the app_server.listen.

private def running_at_background
  extra_space_for_emoji = 1
  (" " * (running_at_message.size + extra_space_for_emoji)).colorize.on_cyan
end

private def running_at
  "http://#{Lucky::ServerSettings.host}:#{Lucky::ServerSettings.port}"
end

private def running_at_message
  "   🎉 App running at #{running_at}   "
end

private def print_running_at
  STDOUT.puts ""
  STDOUT.puts running_at_background
  STDOUT.puts running_at_message.colorize.on_cyan.black
  STDOUT.puts running_at_background
  STDOUT.puts ""
end

print_running_at

app_server.listen

It will always output the correct result.

My app takes 2 minutes to build in development, and commenting out that task makes it take longer.

Yes, actually, commenting out this task probably only add 0.5 seconds within the 2 minutes maybe, but I do think it's good to be able to save this little time is better.

@jwoertink
Copy link
Member Author

if it can't print the port correctly, it shouldn't print it

It does print it correctly if you use it correctly. If you do DEV_PORT=3036 lucky dev it will print 3036. The issue isn't that it is printing incorrectly, the issue is that it's being used incorrectly. This is why I think it's just a documentation error.

image

@zw963
Copy link

zw963 commented Sep 19, 2024

If you do DEV_PORT=3036 lucky dev it will print 3036

Yes, i saw luckyframework/website#1383, it's okay to fix doc for clarify this.

although i consider this is still not the correct way fix this, because:

  1. Pass a DEV_PORT=5000 lucky dev every time start dev server is tired and forgotten easily, this is not a good way to manage port.

  2. Add DEV_PORT=3002 lucky watch to your Procfile.dev is not a good solution too, because this file was added into git.

So, Dotenv is do exactly on this purpose, it never add into git, and avoid user typing it manually, but print wrong port.

@jwoertink
Copy link
Member Author

ok, that's fair. I'll open a new issue for it.

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

No branches or pull requests

2 participants