Change the CLI runners API to just be callables

Now runners consist of just an object that can respond to #call and 
return the exit code.

Old runners are still called with the legacy API but rewriting them with
the new one is encouraged.

CHANGELOG.md

@@ -26,13 +26,24 @@ Whitespace conventions:
 
 ### Added
 
-- Added support for a static folder in the "server" CLI runner via the `OPAL_CLI_RUNNERS_SERVER_STATIC_FOLDER` env var
-- Added ability to pass the port to the "server" CLI runner using the `OPAL_CLI_RUNNERS_SERVER_PORT` (explicit option passed via CLI is still working but depreted)
 - Added `Array#pack` (supports only `C, S, L, Q, c, s, l, q, A, a` formats). (#1723)
 - Added `String#unpack` (supports only `C, S, L, Q, S>, L>, Q>, c, s, l, q, n, N, v, V, U, w, A, a, Z, B, b, H, h, u, M, m` formats). (#1723)
 - Added `File#symlink?` for Node.js. (#1725)
 - Added `Dir#glob` for Node.js (does not support flags). (#1727)
+- Added support for a static folder in the "server" CLI runner via the `OPAL_CLI_RUNNERS_SERVER_STATIC_FOLDER` env var
+- Added ability to pass the port to the "server" CLI runner using the `OPAL_CLI_RUNNERS_SERVER_PORT` (explicit option passed via CLI is still working but deprecated)
+- Added the CLI option `--runner-options` that allows passing arbitrary options to the selected runner, currently the only runner making use of them is `server` accepting `port` and `static_folder`
+
+
+### Changed
+
+- The internal API for CLI runners has changed, now it's just a callable object
+- The `--map` CLI option now works only in conjunction with `--compile` (or `--runner compiler`)
+
+
+### Deprecated
 
+- The CLI `--server-port 1234` option is now deprecated in favor of using `--runner-options='{"port": 1234}'`
 
 
 

lib/opal/cli.rb

@@ -6,8 +6,8 @@
 module Opal
   class CLI
     attr_reader :options, :file, :compiler_options, :evals, :load_paths, :argv,
-                :output, :requires, :gems, :stubs, :verbose, :port, :preload,
-                :filename, :debug, :no_exit, :lib_only
+                :output, :requires, :gems, :stubs, :verbose, :runner_options,
+                :preload, :filename, :debug, :no_exit, :lib_only
 
     class << self
       attr_accessor :stdout
@@ -17,14 +17,12 @@ def initialize options = nil
       options ||= {}
 
       # Runner
-      @runner_type = options.delete(:runner)     || :nodejs
-      @port        = options.delete(:port)       || 3000
+      @runner_type    = options.delete(:runner)         || :nodejs
+      @runner_options = options.delete(:runner_options) || {}
 
       @options     = options
-      @compile     = !!options.delete(:compile)
       @sexp        = options.delete(:sexp)
       @file        = options.delete(:file)
-      @map         = options.delete(:map)
       @no_exit     = options.delete(:no_exit)
       @lib_only    = options.delete(:lib_only)
       @argv        = options.delete(:argv)       || []
@@ -58,24 +56,17 @@ def initialize options = nil
 
     def run
       return show_sexp if @sexp
-
-      compiled_source = builder.to_s
-
-      File.write @map, builder.source_map if @map
-
-      return puts compiled_source if @compile
-
-      runner.run(compiled_source, argv)
-      @exit_status = runner.exit_status
+      @exit_status = runner.call(
+        options: runner_options,
+        output: output,
+        argv: argv,
+        builder: builder,
+      )
     end
 
     def runner
-      @runner ||= begin
-        const_name = @runner_type.to_s.capitalize
-        CliRunners.const_defined?(const_name) or
-          raise ArgumentError, "unknown runner: #{@runner_type.inspect}"
-        CliRunners.const_get(const_name).new(output: output, port: port)
-      end
+      CliRunners[@runner_type] or
+        raise ArgumentError, "unknown runner: #{@runner_type.inspect}"
     end
 
     attr_reader :exit_status
@@ -119,7 +110,7 @@ def show_sexp
         buffer = ::Opal::Source::Buffer.new(filename)
         buffer.source = contents
         sexp = Opal::Parser.default_parser.parse(buffer)
-        puts sexp.inspect
+        output.puts sexp.inspect
       end
     end
 
@@ -150,10 +141,5 @@ def evals_or_file
         end
       end
     end
-
-    def puts(*args)
-      output.puts(*args)
-    end
-
   end
 end

lib/opal/cli_options.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 require 'optparse'
+require 'opal/cli_runners'
 
 module Opal
   class CLIOptions < OptionParser
@@ -78,16 +79,23 @@ def initialize
       end
 
       on('-c', '--compile', 'Compile to JavaScript') do
-        options[:compile] = true
+        options[:runner] = :compiler
       end
 
-      on('-R', '--runner RUNNER', %w[nodejs server applescript nashorn chrome], 'Choose the runner: nodejs (default), server, chrome') do |runner|
+      on('-R', '--runner RUNNER', Opal::CliRunners.to_h.keys, "Choose the runner: nodejs (default), #{(Opal::CliRunners.to_h.keys - [:nodejs]).join(', ')}") do |runner|
         options[:runner] = runner.to_sym
       end
 
-      on('--server-port PORT', 'Set the port for the server runner (default port: 3000)') do |port|
-        options[:runner] = :server
-        options[:port] = port.to_i
+      on('--runner-options JSON', "Set options specific to the selected runner as a JSON string (e.g. port for server)") do |json_options|
+        require 'json'
+        runner_options = JSON.parse(json_options, symbolize_names: true)
+        options[:runner_options] ||= {}
+        options[:runner_options].merge!(runner_options)
+      end
+
+      on('--server-port PORT', '(deprecated, use --runner-options) Set the port for the server runner (default port: 3000)') do |port|
+        options[:runner_options] ||= {}
+        options[:runner_options][:port] = port.to_i
       end
 
       on('-E', '--no-exit', 'Do not append a Kernel#exit at the end of file') do |no_exit|
@@ -120,7 +128,7 @@ def initialize
       end
 
       on('-P', '--map FILE', 'Enable/Disable source map') do |file|
-        options[:map] = file
+        options[:runner_options][:map_file] = file
       end
 
       on('-F', '--file FILE', 'Set filename for compiled code') do |file|

lib/opal/cli_runners.rb

@@ -1,26 +1,81 @@
 # frozen_string_literal: true
 module Opal
-  # `Opal::CliRunners` is the namespace in which JavaScript runners can be
-  # defined for use by `Opal::CLI`. The API for classes defined under
-  # `CliRunners` is the following.
+  # `Opal::CliRunners` is the register in which JavaScript runners can be
+  # defined for use by `Opal::CLI`. Runners will be called via the `#call`
+  # method and passed a Hash containing the following keys:
   #
-  # - The #initialize method takes an `Hash` containing an `output:` object.
-  #   Additional keys can be safely ignored and can be specific to a particular
-  #   runner, e.g. the `CliRunners::Server` runner will accepts a `port:`
-  #   option.
-  # - The runner instance will then be called via `#run(compiled_source, argv)`:
-  #   - `compiled_source` is a string of JavaScript code
-  #   - `argv` is the arguments vector coming from the CLI that is being
+  #
+  # - `options`: a hash of options for the runner
+  # - `output`: an IO-like object responding to `#write` and `#puts`
+  # - `argv`: is the arguments vector coming from the CLI that is being
   #     forwarded to the program
+  # - `builder`: the current instance of Opal::Builder
+  #
+  # Runners can be registered using `#register_runner(name, runner)`.
   #
   module CliRunners
     class RunnerError < StandardError
     end
+
+    @register = {}
+
+    def self.[](name)
+      @register[name.to_sym]
+    end
+
+    def self.[]=(name, runner)
+      warn "Overwriting Opal CLI runner: #{name}" if @register.key? name.to_sym
+      @register[name.to_sym] = runner
+    end
+
+    def self.to_h
+      @register
+    end
+
+    # @param name [Symbol] the name at which the runner can be reached
+    # @param runner [#call] a callable object that will act as the "runner"
+    def self.register_runner(name, runner)
+      self[name] = runner
+      nil
+    end
+
+    # The compiler runner will just output the compiled JavaScript
+    register_runner :compiler, -> data {
+      options  = data[:options] || {}
+      builder  = data.fetch(:builder)
+      map_file = options[:map_file]
+      output   = data.fetch(:output)
+
+      output.puts builder.to_s
+      File.write(map_file, builder.source_map) if map_file
+
+      0
+    }
+
+
+
+    # Legacy runners
+
+    def self.register_legacy_runner(klass_name, *names)
+      runner = -> data {
+        klass = const_get(klass_name)
+        runner = klass.new((data[:options] || {}).merge(output: data[:output]))
+        runner.run(data[:builder].to_s, data[:argv])
+        runner.exit_status
+      }
+      names.each { |name| self[name] = runner }
+    end
+
+    autoload :Applescript, 'opal/cli_runners/applescript'
+    autoload :Chrome,      'opal/cli_runners/chrome'
+    autoload :Nashorn,     'opal/cli_runners/nashorn'
+    autoload :Nodejs,      'opal/cli_runners/nodejs'
+    autoload :Server,      'opal/cli_runners/server'
+
+    register_legacy_runner :Applescript, :applescript, :osascript
+    register_legacy_runner :Chrome,      :chrome
+    register_legacy_runner :Nashorn,     :nashorn
+    register_legacy_runner :Nodejs,      :nodejs, :node
+    register_legacy_runner :Server,      :server
   end
 end
-
-require 'opal/cli_runners/applescript'
-require 'opal/cli_runners/nodejs'
-require 'opal/cli_runners/server'
-require 'opal/cli_runners/nashorn'
-require 'opal/cli_runners/chrome'

spec/lib/cli_spec.rb

@@ -1,9 +1,9 @@
 require 'lib/spec_helper'
 require 'opal/cli'
 require 'stringio'
+require 'tmpdir'
 
 RSpec.describe Opal::CLI do
-  let(:fake_stdout) { StringIO.new }
   let(:file)    { File.expand_path('../fixtures/opal_file.rb', __FILE__) }
   let(:options) { {} }
   subject(:cli) { described_class.new(options) }
@@ -66,14 +66,14 @@
 
   describe ':no_exit option' do
     context 'when false' do
-      let(:options) { {no_exit: false, compile: true, evals: ['']} }
+      let(:options) { {no_exit: false, runner: :compiler, evals: ['']} }
       it 'appends a Kernel#exit at the end of the source' do
         expect_output_of{ subject.run }.to include(".$exit()")
       end
     end
 
     context 'when true' do
-      let(:options) { {no_exit: true, compile: true, evals: ['']} }
+      let(:options) { {no_exit: true, runner: :compiler, evals: ['']} }
       it 'appends a Kernel#exit at the end of the source' do
         expect_output_of{ subject.run }.not_to include(".$exit();")
       end
@@ -82,20 +82,20 @@
 
   describe ':lib_only option' do
     context 'when false' do
-      let(:options) { {lib_only: false, compile: true, evals: [''], skip_opal_require: true, no_exit: true} }
+      let(:options) { {lib_only: false, runner: :compiler, evals: [''], skip_opal_require: true, no_exit: true} }
       it 'appends an empty code block at the end of the source' do
         expect_output_of{ subject.run }.to include("function(Opal)")
       end
     end
 
     context 'when true' do
-      let(:options) { {lib_only: true, compile: true, no_exit: true} }
+      let(:options) { {lib_only: true, runner: :compiler, no_exit: true} }
       it 'appends code block at the end of the source' do
         expect_output_of{ subject.run }.not_to eq("\n")
       end
 
       context 'without any require' do
-        let(:options) { {lib_only: true, compile: true, skip_opal_require: true, no_exit: true} }
+        let(:options) { {lib_only: true, runner: :compiler, skip_opal_require: true, no_exit: true} }
         it 'raises ArgumentError' do
           expect{subject.run}.to raise_error(ArgumentError)
         end
@@ -161,13 +161,29 @@
     end
   end
 
-  describe ':compile option' do
-    let(:options)  { {:compile => true, :evals => ['puts 2342']} }
+  describe ':runner option' do
+    context 'when :compile' do
+      let(:options)  { {runner: :compiler, evals: ['puts 2342']} }
 
-    it 'outputs the compiled javascript' do
-      expect_output_of{ subject.run }.to include(".$puts(2342)")
-      expect_output_of{ subject.run }.not_to include("2342\n")
+      it 'outputs the compiled javascript' do
+        expect_output_of{ subject.run }.to include(".$puts(2342)")
+        expect_output_of{ subject.run }.not_to include("2342\n")
+      end
+
+      context 'with the :map_file runner option' do
+        let(:map_file) { "#{Dir.mktmpdir 'opal-map'}/file.map" }
+        let(:runner_options)  { {map_file: map_file, evals: ['123']} }
+        let(:options) { super().merge(runner_options: runner_options) }
+
+        it 'writes the map file to the specified path' do
+          expect_output_of{ subject.run }.to include(".$puts(2342)")
+          expect_output_of{ subject.run }.not_to include("2342\n")
+          expect(File.read(map_file)).to include(%{"version":3})
+        end
+      end
     end
+
+    # TODO: test more runners
   end
 
   describe ':load_paths options' do
@@ -186,16 +202,6 @@
     end
   end
 
-  describe ':map option' do
-    let(:map_path) { "#{Dir.mktmpdir 'opal-map'}/file.map" }
-    let(:options)  { {map: map_path, evals: ['123']} }
-
-    it 'sets the verbose flag (currently unused)' do
-      expect_output_of{ subject.run }.to eq('')
-      expect(File.read(map_path)).to include(%{"version":3})
-    end
-  end
-
   describe ':parse_comments option' do
     let(:code) do
       <<-CODE
@@ -205,7 +211,7 @@ def m
         end
       CODE
     end
-    let(:options) { { parse_comments: true, evals: [code], compile: true } }
+    let(:options) { { parse_comments: true, evals: [code], runner: :compiler } }
 
     it 'sets $$comment prop for compiled methods' do
       expect_output_of { subject.run }.to include('$$comments = ["# multiline", "# comment"]')
@@ -214,7 +220,7 @@ def m
 
   describe ':enable_source_location' do
     let(:file) { File.expand_path('../fixtures/source_location_test.rb', __FILE__) }
-    let(:options) { { enable_source_location: true, compile: true, file: File.open(file) } }
+    let(:options) { { enable_source_location: true, runner: :compiler, file: File.open(file) } }
 
     it 'sets $$source_location prop for compiled methods' do
       expect_output_of { subject.run }.to include("source_location_test.rb', 6]")
@@ -229,6 +235,7 @@ def expect_output_of
   end
 
   def output_and_result_of
+    fake_stdout = StringIO.new
     stdout = described_class.stdout
     described_class.stdout = fake_stdout
     result = yield