Comments in import blocks

[lencioni]

I've encountered some code that had the import block divided up with comments already. Example:

import Foo from 'foo';
import Bar from 'bar';

// Fruits
import Banana from '../banana';
import Orange from '../orange';

// Animals
import Cat from '../cat';
import Dog from '../dog';

This seems okay to me, but import-js stopped looking for imports once it hit the first comment, which isn't great. I'm not entirely sure what the right thing is to do here, but it seems like what we currently have isn't great. I suppose we could disable grouping in cases like this and then put the import somewhere reasonable--maybe a separate block at the end?

[trotzig]

Yeah, not sure what to do here. Identifying this pattern and putting the imports in a separate block sounds complicated. I think this might be one case where you just have to adapt the import style so that it's consistent with other files. :dragon_face:

[rhettlivingston]

I do not believe that the example shown above is one that we can truly handle because we could never truly determine that a comment applies to multiple lines. We could make a guess based on the combination of the blank line and the comment, but then what would we do? Keep the comment and the whole block together? Auto-disable grouping?

However, I was surprised in another case to discover that comments stopped the scan for imports. My expectation based on automatic code beautifiers I've used in the past (which often had a feature of sorting and grouping includes) was that the comments would be kept and passed through with the statement that they apparently applied to. The assumption is usually made that a comment applies to the statement after it except when the statement appears on the same line.

So I believe that

import hijk from 'localModule'; // trailing comment for local module.
/*
* package leading comment
*/
import cdef from 'package'; // package trailing comment
// CoreModule leading comment
import abcd from 'coreModule';

should become

// CoreModule leading comment
import abcd from 'coreModule';

/*
* package leading comment
*/
import cdef from 'package'; // package trailing comment

import hijk from 'localModule'; // trailing comment for local module.

assuming grouping is enabled.

If grouping is disabled, they should still be tolerated and passed through with the apparently associated package.

The exception would be that comments at the top of the file must remain at the top of the file. They are very possibly instructions for tools like lint. Perhaps an exception to the exception could be made if blank lines were found before the comment and there are no blank lines between the comment and the first import.

[lencioni]

The assumption is usually made that a comment applies to the statement after it except when the statement appears on the same line.

This is usually true, except for cases where you do something like disable a linting rule with a comment, and then re-enable it. e.g.

foo = bar;

/* eslint-disable some-rule */
foo++;
bar++;
/* eslint-enable some-rule */

[rhettlivingston]

Good point. Do you know of cases where that might be done within the imports at the top of the file? I can imagine someone, perhaps, needing to turn off an import plugin resolution check if they don't have an appropriate resolver, but wonder how common that would be.

In any case, I think we should do something like what I outlined as the default.

How much more we do depends on whether there are common cases like that eslint example that we'd expect to appear within imports. If so, we could a) automatically punt on grouping if we detect eslint-disable or eslint-enable directives or b) ignore the case and count on the user to switch to eslint-disable-next or turn off groupImports when we mess up their code.

This does bring up another thought though. There are good reasons why eslint and many other development tools that work on code support those inline directives. This might be a case where a user decides that they don't want us reorganizing their groups in just this one file. Perhaps supporting something like

// import-js { groupImports: false }

should be on our future enhancements list.

[lencioni]

Yeah, not sure how common it is and I agree that we can handle the common cases as they arise.

As for configuration comments, I think that sounds like a reasonable idea. However, I'd like to avoid adding complexity unless it is actually useful, so how about we open an issue to capture the idea and allow people to chime in if they run into the need for it?

[rhettlivingston]

Agreed. I'll open an issue.

I've got a couple of others I need to open related to the Meteor milestone while I'm at it.

I'm considering the Meteor milestone to be what should be done to have a reasonable experience in that environment. It is built largely around the changes necessary to handle the cases encountered in the https://github.com/meteor/todos sample application. I hope to be able to remove most of the imports in that application, importjs fix all of the modules using a fairly generic .importjs.json configuration, and have a working application. The only imports that I believe will have to be manually specified within the modules are the side-effect imports that define the application structure. They are an inherent part of the application design and there are no external clues to their need.

When we hit the milestone, I will "announce" importjs' support for Meteor on the meteor forums and submit a PR to have it included in the Meteor Guide and the meteor/todos sample app.

[rhettlivingston]

Please disregard the meteor milestone turbulence above. Purely accidental.