Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for imports with line-breaks #10

Merged
merged 3 commits into from
Mar 22, 2015
Merged

Add support for imports with line-breaks #10

merged 3 commits into from
Mar 22, 2015

Conversation

trotzig
Copy link
Collaborator

@trotzig trotzig commented Mar 21, 2015

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.

Henric Trotzig added 2 commits March 21, 2015 23:54
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.
I wasn't happy about how this method looked. I'm still not completely
satisfied, there are a few too many things going on. But I've made some
small changes so that things are better grouped and easier to follow.
I've added a few comments too.
@trotzig
Copy link
Collaborator Author

trotzig commented Mar 22, 2015

I'm going to merge this as is. @lencioni those are all valid comments but I don't have time to address right now. Thanks for taking the time to review!

Before this change, they would end up as

  var SomeVariable =~@  require('some_variable');

instead of

  var SomeVariable =
    require('some_variable');

`buffer.append` wasn't dealing with "\n" newlines the way I expected.
This wasn't caught in specs because we use a mock buffer and not a real
one.
trotzig added a commit that referenced this pull request Mar 22, 2015
Add support for imports with line-breaks
@trotzig trotzig merged commit 846bb42 into master Mar 22, 2015
@trotzig trotzig deleted the newlines branch March 22, 2015 20:44
@lencioni
Copy link
Collaborator

Sounds good. I'll send you a PR with these changes then.

lencioni added a commit to lencioni/import-js that referenced this pull request Mar 23, 2015
As discussed in Galooshi#10, we weren't
very happy with how this method reads, so we decided to apply some
changes to make it more understandable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants