Webpage hangs if there's a pending migration

jeremyjung commented 4 years ago

(Not a bug, just a comment on error experience)

After running lucky dev with a pending migration, there is a warning that there is a pending migration. This is great! However the text below it suggests that the app is still running. If you attempt to go to the webpage (localhost:3001) the web browser hangs.

Some potential ideas:

Put the pending migration warning at the bottom of the output so it's clear there's a problem (I missed it until I scrolled up to see it)
Load a webpage that asks you to run migrations
Prompt the user asking them if they would like to run the migration in the CLI or on the webpage (Similar to actionable errors in rails)

lucky dev
system       | Tmux socket name: overmind-sample-app-auth-xmEmUedgZ2fcStbpu9WExg
system       | Tmux session ID: sample-app-auth
system       | Listening at ./.overmind.sock
system_check | Started with pid 64989...
assets       | Started with pid 64991...
web          | Started with pid 64990...
web          | Beginning to watch your project
web          | Compiling...
yarn run v1.22.4
$ yarn run webpack --watch --hide-modules --color --config=node_modules/laravel-mix/setup/webpack.config.js
$ /home/name/Development/sample_app_auth/node_modules/.bin/webpack --watch --hide-modules --color --config=node_modules/laravel-mix/setup/webpack.config.js
assets       | 
assets       | webpack is watching the files…
assets       | 
assets       |  DONE  Compiled successfully in 2226ms                                9:43:13 PM
assets       | 
web          | Done compiling
web          | Unhandled exception: There are pending migrations. Please run lucky db.migrate (Exception)
web          |   from lib/avram/src/avram/migrator/runner.cr:197:7 in 'ensure_migrated!'
web          |   from src/start_server.cr:7:3 in '__crystal_main'
web          |   from /home/name/.asdf/installs/crystal/0.34.0/share/crystal/src/crystal/main.cr:105:5 in 'main_user_code'
web          |   from /home/name/.asdf/installs/crystal/0.34.0/share/crystal/src/crystal/main.cr:91:7 in 'main'
web          |   from /home/name/.asdf/installs/crystal/0.34.0/share/crystal/src/crystal/main.cr:114:3 in 'main'
web          |   from __libc_start_main
web          |   from _start
web          |   from ???
web          | 
web          |                                              
web          |    🎉 App running at http://127.0.0.1:3001   
web          |                                              
web          | 
yarn run v1.22.4
$ /home/name/Development/sample_app_auth/node_modules/.bin/browser-sync start -c bs-config.js --port 3001 -p http://127.0.0.1:5000
web          | [Browsersync] Proxying: http://127.0.0.1:5000
web          | [Browsersync] Access URLs:
web          |  ----------------------------
web          |  Local: http://localhost:3001
web          |  ----------------------------
web          | [Browsersync] Watching files...

paulcsmith commented 4 years ago

Thank you! Yes this is an issue and I think we can solve it by running the pending check inside of a middleware. That way it will show the regular error message screen instead of hanging forever :D

matthewmcgarvey commented 4 years ago

I was looking into how Rails does this and they have a file watcher so that it only runs the check when a migration file has been added. Seems like an ok way to go about it, what do y'all think?

paulcsmith commented 4 years ago

I think the file watcher is a better long term way to do it, but I'm not sure it is worth the extra effort to implement and maintain right now. I think maybe what we can/should do is exit instead of just raising if a pending migration is missing. That way the server never starts at all until the pending migrations are resolved. That might be the simplest way forward for now.

Ideally we'd raise only if a migration was added (like you mentioned) and also have a button on the error page to run pending migrations right away but that'd probably require a pretty large amount of work :S

matthewmcgarvey commented 3 years ago

I believe everything is in place now to efficiently run this check for every request in development. The solution would be to add a middleware handler in Avram and then update lucky_cli to include it for generated apps.

matthewmcgarvey commented 3 years ago

I was thinking that the middleware would access the database info https://github.com/luckyframework/avram/blob/bd5b7eef9368f72f2f109338dfa4380b16561f3c/src/avram/database.cr#L44-L46 but that is memoized and if the migrations are run without restarting the app, we'll never know.

Since we only need to check that migration table against the migrations in the folder, we shouldn't need the database info. One thing I noticed is that we issue a SQL query for each and every migration. Maybe we could refactor to select them all?

luckyframework / lucky

Webpage hangs if there's a pending migration #1142