luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.59k stars 156 forks source link

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

Closed jwoertink closed 3 months ago

jwoertink commented 5 months ago

From this comment: https://github.com/luckyframework/website/issues/1180#issuecomment-2071904360

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 commented 3 months ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 https://github.com/luckyframework/website/issues/1383

zw963 commented 2 weeks ago

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 commented 2 weeks ago

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 :+1:

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

https://github.com/luckyframework/lucky/blob/dd21f8bf9031ed444797534cab07fcbbb149e950/tasks/watch.cr#L220-L222

zw963 commented 1 week ago

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

I will.

https://github.com/luckyframework/lucky/blob/dd21f8bf9031ed444797534cab07fcbbb149e950/tasks/watch.cr#L220-L222

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 commented 1 week ago

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 commented 1 week ago
  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 https://github.com/luckyframework/website/issues/1383

zw963 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

Yes, i saw https://github.com/luckyframework/website/issues/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 commented 1 week ago

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