Skip to content

Commit

Permalink
Add support for imports with line-breaks
Browse files Browse the repository at this point in the history
As reported by @lencioni in #9:

  If I have a section of required modules at the top that looks like this:

    const MyModule = require('path/to/my_module');
    const MyModuleThatHasALongName =
      require('path/to/my_module_that_has_a_long_name');
    const SomeModule = require('path/to/some_module');

  import-js gets confused when adding a new module to the list:

    const AnotherModule = require('path/to/another_module');
    const MyModule = require('path/to/my_module');

    const MyModuleThatHasALongName =
      require('path/to/my_module_that_has_a_long_name');
    const SomeModule = require('path/to/some_module');

  where I would expect:

    const AnotherModule = require('path/to/another_module');
    const MyModule = require('path/to/my_module');
    const MyModuleThatHasALongName =
      require('path/to/my_module_that_has_a_long_name');
    const SomeModule = require('path/to/some_module');

  It would be nice if import-js worked better with this format.

I've added basic support for line breaks in this commit. I say basic
because I expect there to be edge cases that I haven't thought of.
We use simple regexs to parse the js file content, and like all simple
regex parsers, they are bound to fail. However, I don't want to bring in
a js parsing framework just yet. So in this commit I extend the regex
approach with some logic to deal with possible line-breaks.

One idea also listed in #9 was to add support for import-js to
automatically line-break long lines. I haven't made that change in this
commit (new imports are still one-line), but I might do that later if I
find time.
  • Loading branch information
Henric Trotzig committed Mar 21, 2015
1 parent fe82c05 commit 1cc2157
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 18 deletions.
49 changes: 31 additions & 18 deletions ruby/import-js/importer.rb
Expand Up @@ -103,42 +103,55 @@ def window
# @return [number] the number of lines changed
def write_imports(variable_name, path_to_file)
current_imports = find_current_imports
before_length = current_imports.length
current_imports.length.times do

This comment has been minimized.

Copy link
@lencioni

lencioni Mar 21, 2015

Collaborator

It might be nice to add a comment around here explaining your strategy, to make the code a little more readable.

This comment has been minimized.

Copy link
@trotzig

trotzig Mar 22, 2015

Collaborator

I wasn't happy about this method either. I'm not sure a comment will fix that, so instead I'm refactoring it around a little. I'll push another change on top of this commit.

unless buffer[current_imports[:newline_count] + 1].strip.empty?
# Add a newline after imports
buffer.append(current_imports[:newline_count], '')
end

# delete current imports
current_imports[:newline_count].times do
buffer.delete(1)
end

imports = current_imports[:imports]
before_length = imports.length

declaration_keyword = @config['declaration_keyword']
current_imports << "#{declaration_keyword} #{variable_name} = require('#{path_to_file}');"
imports << "#{declaration_keyword} #{variable_name} = require('#{path_to_file}');"

current_imports.sort!.uniq! do |import|
imports.sort!.uniq! do |import|
# Determine uniqueness by discarding the declaration keyword (`const`,
# `let`, or `var`).
import.sub(/\A(const|let|var)\s+/, '')
# `let`, or `var`) and normalizing multiple whitespace chars to single
# spaces.
import.sub(/\A(const|let|var)\s+/, '').sub(/\s\s+/s, ' ')
end

current_imports.reverse.each do |import_line|
# add imports back in
imports.reverse.each do |import_line|
buffer.append(0, import_line)
end

after_length = current_imports.length
unless buffer[current_imports.length + 1].strip.empty?
# Add a newline after imports
buffer.append(current_imports.length, '')
after_length += 1
end
after_length - before_length
imports.length - before_length # truthy if the import was new
end

# @return [Array]
def find_current_imports
lines = []
imports_blob = ''
buffer.count.times do |n|
line = buffer[n + 1]
break unless line.match(/^(const|let|var)\s+.+=\s+require\(.*\).*;\s*$/)
lines << line
break if line.strip.empty?
imports_blob << "\n#{line}"
end
lines

imports = imports_blob.scan(/(?:const|let|var)\s+.+=\s+require\(.*\).*;/)
newline_count = imports.length + imports.reduce(0) do |sum, import|
sum + import.scan(/\n/).length
end
{
imports: imports,
newline_count: newline_count
}
end

# @param variable_name [String]
Expand Down
38 changes: 38 additions & 0 deletions spec/import-js/importer_spec.rb
Expand Up @@ -157,6 +157,27 @@ def self.current_selection=(index)
end
end

context 'when there is an import with line-breaks' do
let(:text) { <<-EOS.strip }
var zoo =
require('foo/zoo');
var tsar = require('foo/bar').tsar;
var foo = { require: b }
EOS

it 'adds the import, sorts the entire list and keeps the line-break' do
expect(subject).to eq(<<-EOS.strip)
var foo = require('bar/foo');
var tsar = require('foo/bar').tsar;
var zoo =
require('foo/zoo');
var foo = { require: b }
EOS
end
end

context 'when there is a blank line amongst current imports' do
let(:text) { <<-EOS.strip }
var zoo = require('foo/zoo');
Expand Down Expand Up @@ -308,6 +329,23 @@ def self.current_selection=(index)
expect(subject).to eq(<<-EOS.strip)
const foo = require('bar/foo');
foo
EOS
end
end

context 'when the import contains a line-break' do
let(:text) { <<-EOS.strip }
var foo =
require('bar/foo');
foo
EOS

it 'changes the `var` to declaration_keyword and removes the whitespace' do
expect(subject).to eq(<<-EOS.strip)
const foo = require('bar/foo');
foo
EOS
end
Expand Down

2 comments on commit 1cc2157

@lencioni
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little on the tired side so I'm having a hard time following along, but it seems 👍 to me. Thanks for working on this!

@trotzig
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a little hacky. My approach here is to make sure that specs are clear and correct and then not worrying too much about the implementation. There's always room for making it nicer later on.

Please sign in to comment.