WEWLJCIO — Working Effectively with Legacy Javascript Code in Opal

I’ve been working on opal-hot-reloader, a hot reloader for Opal applications including built in react.rb support. I merged a pull request (thanks Brock!) that added hot css reloading. The code worked, but it was implemented primarily in Javascript and did not have tests. To make the code easier to read, maintain and extend, I wanted to both add tests and convert it to Ruby/Opal. In particular, I plan to extend the code to handle css served by the Ruby on Rails pipeline so I want to shape the code to be more amenable to extension.

Now if this situation sounds a bit like Working Effectively With Legacy Code by Michael Feathers, it should, and thus the title of this post. This particular situation does present a slightly different set of conditions than many refactorings, i.e.:

  • The desire to convert languages — Javascript to Ruby (or transpiled Ruby via Opal). The interface between those Javascript and Opal objects in the browser affects how things need to proceed.
  • The predefined browser API and test doubles

Hopefully, this article can bring some different insights for Opal developers.

Where to start?

Let’s look at the code. An additional if clause was added to OpalHotReloader#reload() that performs the css hot reloading on the client side. The functionality is implemented in Javascript via an Opal x-string.

def reload(e)
# original code
reload_request = JSON.parse(`e.data`)
if reload_request[:type] == "ruby"
puts "Reloading ruby #{reload_request[:filename]}"
eval reload_request[:source_code]
if @reload_post_callback
@reload_post_callback.call
else
puts "not reloading code"
end
end
# the new css hot reloading code
if reload_request[:type] == "css"
url = reload_request[:url]
puts "Reloading CSS: #{url}"
# Work outsources Javascript via x-string
%x{
var toAppend = "t_hot_reload=" + (new Date()).getTime();
var links = document.getElementsByTagName("link");
for (var i = 0; i < links.length; i++) {
var link = links[i];
if (link.rel === "stylesheet" && link.href.indexOf(#{url}) >= 0) {
if (link.href.indexOf("?") === -1) {
link.href += "?" + toAppend;
} else {
if (link.href.indexOf("t_hot_reload") === -1) {
link.href += "&" + toAppend;
} else {
link.href = link.href.replace(/t_hot_reload=\d{13}/, toAppend)
}
}
}
}
}
  endx
end

First steps

The first thing I noticed was the formerly smallish reload() method had tripled in size and gained a new responsibilty — css hot reloading. I’ll want to refactor that code out the body of the reload() method as a method of a new class, OpalHotReloader::CssReloader dedicated to just css hot reloading (SRP). We’ll instantiate this class instance as @css_reloader in the initializer for OpalHotReloader and then delegate css hot reloading to that instance in OpalHotReloader::CssReloader::reload().

def reload(e)
reload_request = JSON.parse(`e.data`)
if reload_request[:type] == "ruby"
puts "Reloading ruby #{reload_request[:filename]}"
eval reload_request[:source_code]
if @reload_post_callback
@reload_post_callback.call
else
puts "not reloading code"
end
end
if reload_request[:type] == "css"
@css_reloader.reload(reload_request) # extracted method called here
end
end

CssReloader

For our combination of Extract Method and Extract Class, we simply relocated the code to a reload() method of the new class. We don’t have tests yet, so we test it manually. The code still works! Here is the code for the new class.

class OpalHotReloader
class CssReloader
    def reload(reload_request)
url = reload_request[:url]
%x{
var toAppend = "t_hot_reload=" + (new Date()).getTime();
var links = document.getElementsByTagName("link");
for (var i = 0; i < links.length; i++) {
var link = links[i];
if (link.rel === "stylesheet" && link.href.indexOf(#{url}) >= 0) {
if (link.href.indexOf("?") === -1) {
link.href += "?" + toAppend;
} else {
if (link.href.indexOf("t_hot_reload") === -1) {
link.href += "&" + toAppend;
} else {
link.href = link.href.replace(/t_hot_reload=\d{13}/, toAppend)
}
}
}
}
}
end
  end
end

Writing tests

Now that we’ve verified it still works as expected. We will write some tests. We’ll use opal-rspec, which is a popular choice in the opal community and my preferred test framework for Ruby. From here on, I will be referring to the tests as specs since I’m using RSpec.

Because of how we hot reload css via direct maninpulation of stylesheet links in the document, I was faced with the problem of how to set up a test. I could create stylesheet links in the actual DOM of spec runner (fairly easy to do), but I don’t like the idea of calling out to methods the on the global document object. I want to be able to inject a test document for the spec, and inject the real document for the actual application. Fortunately, we can dispense with standard dependency injection (constructor, setter, interface) techniques altogether and just pass the “document” in as a parameter to the method.

Here’s where it gets little tricky. To do hot css reloading we have to manipulate the browser DOM via the global document object. This browser interface is not under my control, so we have to abide by this interface. Because of the interface between Opal objects and Javascript objects (some details described here) we’ll want to create suitable test doubles in pure Javascript. While opal-rspec gives you the full power of rspec’s mock library, these test doubles are Opal objects which won’t act the way the Javascript DOM objects I’m looking to mock/stub out will. We’ll need to create our own method of creating these test doubles.

To create the doubles, I’ve created two methods:

  • create_link() to create the link DOM object that will get altered to facillitate the css hot reloading and
  • fake_links_document() a convenience method which returns both a test double for global document object, which responds to the getElementsByTagName(‘link’) call and a test double for the link itself, that I will inspect to see whether it has been correctly altered.
def create_link( href)
%x|
var ss = document.createElement("link");
ss.type = "text/css";
ss.rel = "stylesheet";
ss.href = #{href};
return ss;
|
end
def fake_links_document(href)
link = create_link(href)
doc = `{ getElementsByTagName: function(name) { links = [ #{link}]; return links;}}`
{ link: link, document: doc}
end

To suport this new “parameter injection” we need to change the existing interface. We change the signature of the OpalHotReloard::CssReloader#reload() to take in the document

# from
def reload(reload_request)
# to
def reload(reload_request, document)

Then we call it with the new signature.

# in OpalHotReloader#reload()
# instead of calling it this way
@css_reloader.reload(reload_request)
# we pass in the real browser document
@css_reloader.reload(reload_request, `document`)

The class at this point looks like this

class OpalHotReloader
class CssReloader
    def reload(reload_request, document) #  pass in the "document"
url = reload_request[:url]
%x{
var toAppend = "t_hot_reload=" + (new Date()).getTime();
// invoke it here
var links = #{document}.getElementsByTagName("link");
for (var i = 0; i < links.length; i++) {
var link = links[i];
if (link.rel === "stylesheet" && link.href.indexOf(#{url}) >= 0) {
if (link.href.indexOf("?") === -1) {
link.href += "?" + toAppend;
} else {
if (link.href.indexOf("t_hot_reload") === -1) {
link.href += "&" + toAppend;
} else {
link.href = link.href.replace(/t_hot_reload=\d{13}/, toAppend)
}
}
}
}
}
end
  end
end

We test out the new interface and it still works. Yay!

Writing specs with the new interface

Now we can write specs and inject our own test doubles for document. There are 3 cases we want to prove are handled correctly:

  • A plain stylesheet link where we add the hot reload argument for the first time.
  • Updating a link that has already been updated with a hot reload argument.
  • Appending an additional hot reload argument parameter to a stylesheet link that already has a parameter.
require 'native'
require 'opal_hot_reloader'
require 'opal_hot_reloader/css_reloader'
describe OpalHotReloader::CssReloader do
  def create_link( href)
%x|
var ss = document.createElement("link");
ss.type = "text/css";
ss.rel = "stylesheet";
ss.href = #{href};
return ss;
|
end
  def fake_links_document(href)
link = create_link(href)
doc = `{ getElementsByTagName: function(name) { links = [ #{link}]; return links;}}`
{ link: link, document: doc}
end

  context 'Rack::Sass::Plugin' do
it 'should add t_hot_reload to a css path' do
css_path = 'stylesheets/base.css'
doubles = fake_links_document(css_path)
link = Native(doubles[:link])
expect(link[:href]).to match /#{Regexp.escape(css_path)}$/
subject.reload({ url: css_path}, doubles[:document])
expect(link[:href]).to match /#{Regexp.escape(css_path)}\?t_hot_reload=\d+/
end
    it 'should update t_hot_reload argument if there is one already' do 
css_path = 'stylesheets/base.css?t_hot_reload=1111111111111'
doubles = fake_links_document(css_path)
link = Native(doubles[:link])
expect(link[:href]).to match /#{Regexp.escape(css_path)}$/
subject.reload({ url: css_path}, doubles[:document])
expect(link[:href]).to match /#{Regexp.escape('stylesheets/base.css?t_hot_reload=')}(\d)+/
expect($1).to_not eq '1111111111111'
end
    it 'should append t_hot_reload if there are existing arguments' do
css_path = 'stylesheets/base.css?some-arg=1'
doubles = fake_links_document(css_path)
link = Native(doubles[:link])
expect(link[:href]).to match /#{Regexp.escape(css_path)}$/
subject.reload({ url: css_path}, doubles[:document])
expect(link[:href]).to match /#{Regexp.escape(css_path)}\&t_hot_reload=(\d)+/
end
end
end

Specs pass — Safe to refactor

Now that we have test coverage for the 3 cases, we can now rewrite the reload() method in Ruby/Opal. The safety net of the newly writen specs will ensure that we do it correctly. For development, it will be handy to have both versions code side by side both for coding and testing. I did a little trick of having Javascript and Ruby versions of reload() and then having the reload() call either of those as needed. The code looked like this.

require 'native'
class OpalHotReloader
class CssReloader
    def reload(reload_request, document)
# currently using the Ruby version
reload_ruby(reload_request, document)
# reload_js(reload_request, document)
end
    def reload_ruby(reload_request, document)
url = reload_request[:url]
puts "Reloading CSS: #{url}"
to_append = "t_hot_reload=#{Time.now.to_i}"
links = Native(`document.getElementsByTagName("link")`)
(0..links.length-1).each { |i|
link = links[i]
if link.rel == 'stylesheet' && link.href.index(url)
if link.href !~ /\?/
link.href += "?#{to_append}"
else
if link.href !~ /t_hot_reload/
link.href += "&#{to_append}"
else
link.href = link.href.sub(/t_hot_reload=\d{13}/, to_append)
end
end
end
}
end
    def reload_js(reload_request, document)
url = reload_request[:url]
%x{
var toAppend = "t_hot_reload=" + (new Date()).getTime();
var links = #{document}.getElementsByTagName("link");
for (var i = 0; i < links.length; i++) {
var link = links[i];
if (link.rel === "stylesheet" && link.href.indexOf(#{url}) >= 0) {
if (link.href.indexOf("?") === -1) {
link.href += "?" + toAppend;
} else {
if (link.href.indexOf("t_hot_reload") === -1) {
link.href += "&" + toAppend;
} else {
link.href = link.href.replace(/t_hot_reload=\d{13}/, toAppend)
}
}
}
}
}
end
  end
end

We implement the Ruby version, a line by line translation, and the specs pass. Now that the Ruby version works, we don’t need the Javascript version. We can remove the unnecessary code.

require 'native'
class OpalHotReloader
class CssReloader
    def reload(reload_request, document)
url = reload_request[:url]
puts "Reloading CSS: #{url}"
to_append = "t_hot_reload=#{Time.now.to_i}"
links = Native(`document.getElementsByTagName("link")`)
(0..links.length-1).each { |i|
link = links[i]
if link.rel == 'stylesheet' && link.href.index(url)
if link.href !~ /\?/
link.href += "?#{to_append}"
else
if link.href !~ /t_hot_reload/
link.href += "&#{to_append}"
else
link.href = link.href.sub(/t_hot_reload=\d{13}/, to_append)
end
end
end
}
end

  end
end

Now I’m ready to be able to extend this class to support css from Rails’ asset pipeline!

Thanks to Scott Smith for proofreading this.

Article originally published here check my blog for more articles on software development.