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 newline-after-import rule #245

Closed
wants to merge 2 commits into from
Closed

Add newline-after-import rule #245

wants to merge 2 commits into from

Conversation

radekbenkel
Copy link
Contributor

@radekbenkel radekbenkel commented Apr 18, 2016

newline-after-import (#244)

Reports if there's no new line after last import in group.

Rule Details

Valid:

import defaultExport from './foo'

const FOO = 'BAR'
import defaultExport from './foo'
import { bar }  from 'bar-lib'

const FOO = 'BAR'

...whereas here imports will be reported:

import * as foo from 'foo'
const FOO = 'BAR'
import * as foo from 'foo'
const FOO = 'BAR'

import { bar }  from 'bar-lib'

When Not To Use It

If you like to visually group module imports with its usage, you don't want to use this rule.

@jfmengels
Copy link
Collaborator

Looks good to me, but is it possible to also support require() at the top of the document? Makes it quite a bit more complicated, but I know I (and the one who requested #244, cc @sindresorhus) would like to have that.

@radekbenkel
Copy link
Contributor Author

radekbenkel commented Apr 18, 2016

@jfmengels You mean, you would like it to work for import foo from 'foo'
and/or
var foo = require('foo')?

@sindresorhus
Copy link

@singles Yes :)

@@ -48,6 +48,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Ensure all imports appear before other statements ([`imports-first`])
* Report repeated import of the same module in multiple places ([`no-duplicates`])
* Report namespace imports ([`no-namespace`])
* Enforce new line after import not followed by another import([`newline-after-import`])

Choose a reason for hiding this comment

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

Maybe:

Enforce a newline after import statements ([`newline-after-import`])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx, fixed :)

@radekbenkel
Copy link
Contributor Author

radekbenkel commented Apr 18, 2016

@sindresorhus @jfmengels thank you guys for feedback.

Regarding require. I didn't add support for require after reading docs, which says that This plugin intends to support linting of ES2015+ (ES6+) import/export syntax.

Also I checked test folder for usage of require and none of the current rules support that, while you could think that for example imports-first should do that, because require is still an import, just other syntax.

Please don't get me wrong - I'm not trying to be cocky or something, it just matter of consistency for me :)

EDIT Of course I can add support for require if you're ok with what I described above :)

@jfmengels
Copy link
Collaborator

You're right that this project doesn't support require. There are some rules coming in from another project which has rules that does support require statements.

From what I understood, this plugin supports rules that can infer something from static analysis using import. Using require, that becomes harder/not possible, and therefore, most rules can't be applied to uses of require (might be wrong, haven't had the chance to look at this plugin in too much detail). In this case (and the rule I mentioned above), it is simply a stylistic issue (which are new to the project too), and can be applied to both "methods".

@benmosher Thoughts on supporting require where possible (and first of all, here)?

@benmosher
Copy link
Member

FWIW, no-unresolved does support both CommonJS and AMD module syntax, if you are using Browserify or Webpack. Could theoretically support other AMD module loaders if the resolvers were implemented.

For that rule, you may supply an options object like { commonjs: true, amd: true } to enable one or both.

I think it makes sense to support CommonJS to the extent that

  • the community wants it
  • it's technically feasible

I think that is true on both counts in this case.

@radekbenkel
Copy link
Contributor Author

radekbenkel commented Apr 19, 2016

Now when #247 is here and based on comments from #246 I think it would be nice to solve this as a part of addtion to #247. Just additional option called newline-after-import-group, which will give you empty line before any other code, even if you will create one group with all import types.

What do you think?

@benmosher
Copy link
Member

Tough call. I might enable this rule without worrying about ordering within the imports themselves, so I'm not sure it makes sense rolled into order. (@sindresorhus convinced me with a similar argument in discussion on #253)

I think this makes sense as you've implemented it for imports, and I think the require version would just ensure that the next line is either another require call (possibly an assignment, or could be side-effects only) or a blank line, i.e.

const x = require('x')
require('jqueryui')
var y = require('y')

// ^ line above is enforced
function boringName(someArg) {
  return new Error("the worst function ever") 
}

let z = require('ugh-mutable-module-reference-ftl')

// ^ another enforced line here
function stahp() {
  process.exit(-)
}

No need to enforce on require calls except at module scope†, so you could implement with a Program node visitor with a loop over program.body.

†: unless @jfmengels or @sindresorhus say otherwise. I don't think it's important for v1 but I'm not going to use it, so what do I know 😅

I guess you could provide the same visitor for both Program and BlockStatement nodes, they each have a body field that is an array of Statements.

@radekbenkel
Copy link
Contributor Author

OK, rebased onto master and added support for require :)

ensureNoForbiddenKeyword(context, node, nextToken, 'import')
},
CallExpression: function(node) {
if (node.callee.name === "require") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mind using single-quotes here? For consistency with the other strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, also see #280 :)

@jfmengels
Copy link
Collaborator

LGTM :)

@radekbenkel
Copy link
Contributor Author

@benmosher no problem, done.

@benmosher
Copy link
Member

Rebased and added changelog updates. 👍 it's in master, ready for publish via 1.7.0. Not sure when that will be, probably want to get some of @sindresorhus's issues resolved first.

@benmosher benmosher closed this Apr 29, 2016
@benmosher benmosher added this to the next milestone Apr 29, 2016
@jfmengels
Copy link
Collaborator

@benmosher It's not very nice of you to say that @sindresorhus has issues :p

@sindresorhus
Copy link

It's true. I have a gadzillion issues...

@jfmengels
Copy link
Collaborator

jfmengels commented Apr 29, 2016

You made me found out I had way more issues than I thought... If it makes you feel better, I have a 3.5 hour trip, and I'll spend at least some of it solving your issues ^^

Edit: No, it's fine, most of them are greenkeeper PRs on a project I have abandoned :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants