New amber watch config #865

faustinoaq · 2018-06-16T18:24:50Z

Description of the Change

This PR depends on #864 and #857

This PR adds new amber watch config using .amber.yml file

Alternate Designs

Keep current process_runner.cr and force npm/webpack commands 😅

Benefits

Fixes [CLI] Amber Watch - Configurable #476
Fixes [CLI] Amber watch starts multiple processes #507
Allow to add watch tasks
Allow to add one-time tasks
Allow to customize build and run commands
Allow to customize client files to watch
Removes sentry.cr (dead code)

Possible Drawbacks

All tasks require files and commands fields, for one-time tasks (without files) you can use an empty array files: []

No cli flags, so, things like amber watch -w "./config/**/*.cr" are not supported /cc @damianham

See: https://github.com/damianham/amber_render_module#usage

faustinoaq · 2018-06-16T18:29:12Z

This PR is based on new-exception-page branch, see: #864

…/amber into fa/new-amber-watch-config

robacarp · 2018-06-19T22:31:51Z

src/amber/cli/templates/app/.amber.yml.ecr

+      - "crystal build -o bin/<%= @name %> src/<%= @name %>.cr -p --no-color"
+      - "bin/<%= @name %>"
+
+  client: # required: these files changes trigger browser reloading


should this be required? I generally do almost no front end code, what if I don't want watch to do anything?

@robacarp You're right, actually this field can be optional, this implementation works without a client field

faustinoaq · 2018-06-20T20:55:02Z

This new config works pretty nice and fix many issues, although, Does someone have any idea how to add specs for this? 😅

robacarp

There are a lot of special cases in here for the server and client watcher, but it seems like that shouldn't be necessary. The one switch that I can see which needs to be made is simply: run a command on the server, or send a message down a channel. What about changing the way the configuration is written to be more generic?

watch:
    meaningless_name:
      files:
        - src/**/*.cr
        - # etc
      notify_client: true   # sends a message through the channel
      commands:
         - shards build app # every command treated the same, and run in parallel

Would it clean up the watcher code and config?

robacarp · 2018-06-21T15:49:32Z

src/amber/cli/helpers/process_runner.cr

-      loop do
-        scan_files
-        sleep 1
+      if watch_object = CLI.config.watch


Is watch_config a better name than watch_object?

robacarp · 2018-06-21T15:50:35Z

src/amber/cli/helpers/process_runner.cr

+      if watch_object = CLI.config.watch
+        run_watcher(watch_object)
+      else
+        error "Can't find watch settings, please check your .amber.yml file"


Can this error message be more helpful? Better yet, can this be a warning and then some sane default watch run anyway?

Yeah, nice idea, I'm already working on that 👍

robacarp · 2018-06-21T15:54:11Z

src/amber/cli/helpers/process_runner.cr

-          log "Compile time errors detected. Shutting down..."
-          exit 1
+    private def run_watcher(watch_object)
+      watch_object.each do |key, value|


I'm not sure it makes sense to iterate here. Why not just do something like:

server_config = watch_object["server"] spawn watcher("server", server_config["files"], server_config["commands"]

robacarp · 2018-06-21T15:59:56Z

src/amber/cli/helpers/process_runner.cr

+        debug "Watching #{file_counter} #{key} files"
+        kill_processes(key)
+        commands.each do |command|
+          if key == "server" && command == commands.first?


this logic isn't obvious to me, why is the first command different from the rest?

This means that other tasks are blocked until your amber app compiles,

Should we change that behaivior? 😅

I think your idea of running task on parallel would be nice 👍

robacarp · 2018-06-21T16:00:39Z

src/amber/cli/helpers/process_runner.cr

-      @processes.each do |process|
-        process.kill unless process.terminated?
+    private def check_directories
+      Dir.mkdir_p("bin")


why does this create bin/?

Because bin/ dir is needed to store the generated binary

Useful when your tasks are like crystal build src/myapp.cr -o bin/myapp, so you don't need to execute mkdir -p first because amber watch already does this, just like shards build

robacarp · 2018-06-21T16:01:31Z

src/amber/cli/helpers/process_runner.cr

+        process = Helpers.run(command, error: error_io)
+        PROCESSES << {process, "server"}
+        loop do
+          if process.terminated?


I think this loop contains far too much logic all in one place. It may help to refactor to use early-exits instead, but I don't know. It's hard to even see what's going on here, but something like this might be possible:

loop do sleep 1 next if process.terminated? next if error_io.empty? # etc end

I added a new method handle_terminaded_process:

Now the loop looks like this:

loop do if process.terminated? handle_terminaded_process(process, error_io) break end sleep 1 end

eliasjpr · 2018-06-21T16:15:19Z

src/amber/cli/templates/app/.amber.yml.ecr

+      - "crystal build -o bin/<%= @name %> src/<%= @name %>.cr -p --no-color"
+      - "bin/<%= @name %>"
+
+  client: # optional: these files changes trigger browser reloading


# Optional: File changes triggers the browser to reload.

eliasjpr · 2018-06-21T16:16:46Z

src/amber/cli/templates/app/.amber.yml.ecr

+      - "public/**/*"
+    commands: []
+
+  webpack: # optional: compiles assets using webpack


I thought the new Amber watch was going to decouple webpack meaning you can choose which ever front end builder you want? Or is this a simple task?

This is just a simple task, Webpack removal is addressed here: #866

eliasjpr · 2018-06-21T16:17:37Z

src/amber/cli/helpers/process_runner.cr

-      File.info(file).modification_time.to_s("%Y%m%d%H%M%S")
+    private def handle_error(error_output)
+      kill_processes("server")
+      puts error_output


User CLI::Logger

eliasjpr · 2018-06-21T16:18:09Z

src/amber/cli/templates/app/.amber.yml.ecr

@@ -2,3 +2,27 @@ type: app
 database: <%= @database %>
 language: <%= @language %>
 model: <%= @model %>
+watch:
+  server: # required: the first command for this task is blocking


What does this mean required: the first command for this task is blockin?

That means that other tasks are blocked until your amber app compiles

Maybe we should change the message, WDYT? 😅

faustinoaq · 2018-06-21T19:42:29Z

There are a lot of special cases in here for the server and client watcher, but it seems like that shouldn't be necessary. The one switch that I can see which needs to be made is simply: run a command on the server, or send a message down a channel. What about changing the way the configuration is written to be more generic?

@robacarp Nice idea, let me try 👍

faustinoaq · 2018-06-21T19:49:22Z

@robacarp I have a comment about your idea:

watch:
    meaningless_name:
      files:
        - src/**/*.cr
        - # etc
      notify_client: true   # This is already done by "client" field
      commands:
         - shards build app # Should we run this on parallel? what about next command
         - bin/app               # This requires app build first, WDYT?

faustinoaq · 2018-06-21T19:51:07Z

      notify_client: true   # Nice, idea, this replaces "client" field

I got it!, I think I can implement this by appending all fields (meaningless_name) files with notify_client: true to the client watcher 👍

faustinoaq · 2018-06-21T20:03:03Z

      commands:
         - shards build app # Should we run this on parallel? what about next command
         - bin/app               # This requires app build first, WDYT?

If we use parallel tasks, then I guess we should require something like this:

      commands:
         - shards build app && bin/app

WDYT?

eliasjpr · 2018-06-21T20:42:37Z

@faustinoaq I would say not to mess with the feature too much, and iterate i will open issues with additional thoughts so we can merge this

faustinoaq · 2018-06-21T21:55:40Z

@eliasjpr Yeah, no problem, now v0.8.0 is already released by @robacarp , so I'll fix this feature for v0.8.1 👍

I just realize @robacarp idea is simpler and nicer 😅

eliasjpr

I left some comments. I feel that the ProcessRunner can bit split into smaller structs/classes its doing too much and know too much about Watcher.

eliasjpr · 2018-06-22T12:36:32Z

src/amber/cli/commands/database.cr

@@ -55,7 +55,7 @@ module Amber::CLI
          when "create"
            Micrate.logger.info create_database
          when "seed"
-            Helpers.run("crystal db/seeds.cr", wait: true, shell: true)
+            Helpers.run("crystal db/seeds.cr", shell: true).wait


Helpers is a module and the .wait call makes it look like is an instance of an object and the method .run is deceiving since its just creating an instance of Process. I like that this method has some helpful arguments defaults and in that case I recommend to use class CLIProcess < Process and overwrite the run method with defaults and call super on it.

require "process" class CLIProcess CLI_IO = Process::Redirect::Inherit def self.instance(command, shell = true, input = CLI_IO, output = CLI_IO, error = CLI_IO) Process.new(command, shell: shell, input: input, output: output, error: error) end end

CLIProcess.instance("crystal db/seeds.cr", shell: true).wait

Looks cleaner, Nice idea! 👍

eliasjpr · 2018-06-22T12:51:12Z

src/amber/cli/config.cr

+          "bin/#{app_name}"
+        ]
+      }
+    }

    def initialize


You can remove this initializer since is not doing anything

If I remove the initialize then I can't use Config.new because it complains about no matching parameters 😅

Let me add a comment about this 👍

eliasjpr · 2018-06-22T13:05:47Z

src/amber/cli/helpers/process_runner.cr

+        error "Error in watch configuration. #{ex.message}"
+        exit 1
+      end
+      watch_config.each do |key, value|


This should be its own method and be able to loop to all config keys including watch_config["server"]

I applied @robacarp suggestion here, although, the new refactoring won't require this 😅

eliasjpr · 2018-06-22T13:07:13Z

src/amber/cli/helpers/process_runner.cr

+    private def run_watcher(watch_config)
+      begin
+        server_config = watch_config["server"]
+        spawn watcher("server", server_config["files"], server_config["commands"])


Why is server is being spawn separately and not just loop over each key as you doing below? Does the order matter? if yes, this should be moved to its own method run_server_watcher

The watch_config should be an instance variable!

I applied @robacarp suggestion here, although, the new refactoring won't require this 😅

eliasjpr · 2018-06-22T13:08:59Z

src/amber/cli/helpers/process_runner.cr

-        elsif !@app_running
-          log "Compile time errors detected. Shutting down..."
-          exit 1
+      if watch_config = CLI.config.watch


This should be moved to initializer as a guard clause. watch

def initialize raise AMBER_YAML_ERROR unless CLI.config.amber_yml_exists? @amber_yaml = CLI.config.amber_yaml end

Actually the loading and check of this should be in Amber::CLI module and not here.

And have an instance of ProcessRunner in Amber::CLI

module Amber::CLI def self.process_runner ProcessRunner.new if amber_yml_loaded? end def amber_yaml_loaded? raise AMBER_YAML_ERROR unless CLI.config.amber_yml_exists? end end

Nice suggestion, let me apply this refactoring as well 👍

eliasjpr · 2018-06-22T13:19:21Z

src/amber/cli/helpers/process_runner.cr

-    private def build_app_process
-      log "Building project #{project_name}..."
-      Amber::CLI::Helpers.run(@build_command)
+    private def handle_terminaded_process(process, error_io)


Why not a TerminatedProcessHandler classs?

I think this useless now, I gonna use parallel processes (like @robacarp suggested), no channel nor concurrency management would be required 👍

eliasjpr · 2018-06-22T13:22:39Z

src/amber/cli/helpers/process_runner.cr

+      end
+    end
+
+    private def error_server(error_output)


The body of this method should be moved to a ErrorServer class/module

I agree, let me split this file 👍

eliasjpr · 2018-06-22T13:23:00Z

src/amber/cli/helpers/process_runner.cr

+
+    private def handle_error(error_output)
+      kill_processes("server")
+      puts error_output


CLI::Logger?

Oh, I forgot this one 😅

eliasjpr · 2018-06-22T13:26:53Z

src/amber/cli/helpers/process_runner.cr

+      @notify_counter = 0
+      @notify_counter_channel = Channel(Int32).new
+      @notify_channel = Channel(Nil).new
+      @host = Helpers.settings.host


How come we are getting settings information from a Helper module and not Amber::CLI environment settings?

Oh, this is using include Enviroment, I know this is a bad practice, because even I was confused about this when I tried to figure out where Helpers.settings is defined, So, I gonna use Amber::CLI 👍

eliasjpr · 2018-06-22T13:27:56Z

src/amber/cli/helpers/process_runner.cr

-      Amber::CLI::Helpers.run("npm install --loglevel=error && npm run watch", wait: false)
-      node_log "Watching public directory"
+    private def run_command(command, key)
+      if key == "server"


Why is "server" special?

Yeah, I did such a complex stuff here, I'm already fixing this 👍

drujensen · 2018-10-06T19:23:28Z

@robacarp Why was this closed? I'm confused as to the state of this issue.

elorest · 2018-10-06T20:16:33Z

Why was this closed? Looks a little complex, but there's some good stuff in here.

@faustinoaq Will there be another PR with some of this? Also will autoreload be added back soon?

robacarp · 2018-10-06T22:48:52Z

@elorest @drujensen This was closed for cleanup -- @faustinoaq did some great work, but doesn't have time these days to contribute as much. If someone wants to pick up where he left of, 👍

faustinoaq added 5 commits June 16, 2018 10:19

Added new exception page

8abe79c

Simplify Helpers.run

994e730

Add new amber watch config

7925382

Add new watch config

d148ce9

Add JavaScript to support reload on html pages using default layout.

b65e3fb

faustinoaq requested review from marksiemers, eliasjpr and robacarp June 16, 2018 18:24

faustinoaq added kind:cli kind:enhancement kind:feature pr:needs-review labels Jun 16, 2018

faustinoaq mentioned this pull request Jun 16, 2018

Remove webpack #866

Closed

faustinoaq requested a review from a team June 16, 2018 18:41

faustinoaq requested a review from a team June 16, 2018 18:42

Add missing exception

7f49bd7

This was referenced Jun 16, 2018

Remove reload pipe #860

Merged

Change Pipe::Reload to amber_reload.js amberframework/recipes#26

Merged

faustinoaq and others added 4 commits June 17, 2018 12:06

Fix typo

19dc8d6

Replace mtime by modification_time

10e9f60

Merge branch 'fa/new-amber-watch-config' of github.com:amberframework…

4995c92

…/amber into fa/new-amber-watch-config

Remove trailing whitespaces

65f81aa

robacarp reviewed Jun 19, 2018

View reviewed changes

faustinoaq added 4 commits June 19, 2018 18:04

Client field is optional

36f7b3a

Use more descriptive var name

11a014f

Merge branch 'master' into fa/new-amber-watch-config

dbd3163

Merge branch 'master' into fa/new-exception-page

20d9892

faustinoaq mentioned this pull request Jun 20, 2018

Add new amber watch configuration to all recipes amberframework/recipes#28

Open

robacarp reviewed Jun 21, 2018

View reviewed changes

eliasjpr reviewed Jun 21, 2018

View reviewed changes

faustinoaq added 5 commits June 21, 2018 14:24

Add reload pipe again (still need to generate websocket reload server)

25443c3

Fix layout template

080f96c

Add config generator

0521e1d

Add handle_terminaded_process

a3167ed

Use Titlecase

b88fdc2

eliasjpr reviewed Jun 22, 2018

View reviewed changes

Use tryReload()

ff5f794

drujensen mentioned this pull request Jul 5, 2018

Add API-only CLI option #903

Closed

robacarp closed this Aug 24, 2018

This was referenced Oct 14, 2018

added reload pipe and specs back #960

Closed

removed INJECTION const that when injected affected certain styles on… #959

Merged

drujensen reopened this Nov 28, 2018

drujensen closed this Dec 24, 2018

drujensen deleted the fa/new-amber-watch-config branch December 24, 2018 01:58

New amber watch config #865

New amber watch config #865

Conversation

faustinoaq commented Jun 16, 2018 • edited Loading

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

faustinoaq commented Jun 16, 2018

Choose a reason for hiding this comment

faustinoaq Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

faustinoaq commented Jun 20, 2018 • edited Loading

robacarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustinoaq Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustinoaq commented Jun 21, 2018 • edited Loading

faustinoaq commented Jun 21, 2018

faustinoaq commented Jun 21, 2018 • edited Loading

faustinoaq commented Jun 21, 2018 • edited Loading

eliasjpr commented Jun 21, 2018

faustinoaq commented Jun 21, 2018

eliasjpr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustinoaq Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustinoaq Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drujensen commented Oct 6, 2018

elorest commented Oct 6, 2018

robacarp commented Oct 6, 2018

faustinoaq commented Jun 16, 2018 •

edited

Loading

faustinoaq Jun 19, 2018 •

edited

Loading

faustinoaq commented Jun 20, 2018 •

edited

Loading

faustinoaq Jun 21, 2018 •

edited

Loading

faustinoaq commented Jun 21, 2018 •

edited

Loading

faustinoaq commented Jun 21, 2018 •

edited

Loading

faustinoaq commented Jun 21, 2018 •

edited

Loading

faustinoaq Jun 22, 2018 •

edited

Loading

faustinoaq Jun 22, 2018 •

edited

Loading