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

Enforce import index style #9

Closed
sindresorhus opened this issue Apr 18, 2016 · 17 comments
Closed

Enforce import index style #9

sindresorhus opened this issue Apr 18, 2016 · 17 comments

Comments

@sindresorhus
Copy link
Owner

I want to enforce require('.'); and from '.';. Before Node.js 4 you had to do './'. In Node.js 4 you can do: '.'.

Should catch the following cases:

  • './'
  • './index'
  • './index.js'

Should be easy to implement auto-fixing for this rule.

@sindresorhus
Copy link
Owner Author

I don't see the point in an option. 0.12 is already being phased out. It went into maintenance mode early this month. I'd rather have a simple rule without any options that we can add to esnext in XO for now.

@jamestalmage
Copy link
Contributor

My thought is that most of what I work on still supports 0.12, and will need to for the next 12 months.

. vs ./ isn't that big of deal, but it would be nice to call out ./index and ./index.js.

Either way, the rule should also handle relative references to index.js:

  • ../../index.js -> ../../

@sindresorhus
Copy link
Owner Author

Alright. So how about we fail on ./index and ./index.js and allow . and ./, but in a year we change it to only .?

@jfmengels
Copy link
Contributor

If it gets in XO with esnext and not as the default, I don't see why it couldn't be . already. Or give it an option to prefer ./ or ., that way, we won't have to change it in a year and "migrate" the codebases that use esnext.

@sindresorhus
Copy link
Owner Author

Or give it an option to prefer ./ or ., that way, we won't have to change it in a year and "migrate" the codebases that use esnext.

I was hoping to not need an option, but I agree with @jfmengels. That sounds like the best solution. Should default to ., so we can just remove the option in a year.

@ayrton
Copy link

ayrton commented Apr 25, 2016

I wrote this earlier today for closed project, happy to open a PR:

module.exports = function(context) {
  return {
    'ImportDeclaration': function(node) {
      if (node.source.value.slice(-1) === '/') {
        context.report(node, 'Omit trailing slashes in your imports. Rename to `' + node.source.value.slice(0, -1) + '`');
      }

      if (node.source.value.match(/\/index\.js/)) {
        var index = node.source.value.indexOf('/index.js')
        context.report(node, 'Omit `/index.js` in your imports. Rename to `' + node.source.value.slice(0, index) + '`');
      }
    }
  }
}

@jfmengels
Copy link
Contributor

@ayrton Great! And yes, please make a PR :)

A few remarks already though:

  • We would like support for require too
  • You don't handle the case of ./index

Thanks for doing this!

@sindresorhus
Copy link
Owner Author

sindresorhus commented Apr 25, 2016

That would be great! :)

Also needs to handle require().

@jfmengels
Copy link
Contributor

@ayrton Are you still up for making a PR for this? No worries if you are not.

@ayrton
Copy link

ayrton commented Aug 2, 2016

@jfmengels I'm currently on holiday - so wouldn't mind if someone took over. Won't be able to work on it for another two weeks.

@jfmengels
Copy link
Contributor

@ayrton Sure, no problem. Maybe I'll tackle this, maybe I won't, you'll see when you come back 😄 Enjoy the holidays!

Now that I think about it and that we've worked with eslint-plugin-import a bit, I think this would be a better fit there. What do you think @benmosher?

@benmosher
Copy link

Sounds like: import-js/eslint-plugin-import#394 which I'm up for.

I skimmed this thread, but is what you're looking for applicable only to . or also ./foo?

@jfmengels
Copy link
Contributor

Hah. I actually 👍'd that one. Totally slipped my mind.

Originally, this issue is only for the index of the current folder.

Now that you mention it, the same will be applicable to the parent (or its parent, etc.) .. vs ../.

I'd make this more generic and also apply it to ./foo, to prevent ./foo/, ./foo/index or ./foo/index.js, but the index (also parent index?) is a bit special as . is not valid in 0.12.

@benmosher
Copy link

I'd be inclined not to support 0.12 since it's August, now. The majority of this thread is from before 0.12 phased into maintenance mode, IIRC. I could go either way, though. Would probably want the default to be 0.12-ignorant and put ./+../ exceptions behind a flag, if anything, just for consistency.

@sindresorhus
Copy link
Owner Author

sindresorhus commented Aug 2, 2016

No point in supporting nor having an option for 0.12 at this point. Really! Even ESLint has dropped support for it.

@jfmengels
Copy link
Contributor

jfmengels commented Aug 2, 2016

True. And if it does, they can disable this rule. 👍

@sindresorhus Does that mean you'll start writing packages in Node v4 ES6 code from now on? Or soon?

@sindresorhus
Copy link
Owner Author

@jfmengels I've been writing new modules with Node.js 4 as target for a while now.

SamVerschueren added a commit to SamVerschueren/eslint-plugin-unicorn that referenced this issue Jul 10, 2017
SamVerschueren added a commit to SamVerschueren/eslint-plugin-unicorn that referenced this issue Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants