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 absolute imports to import/order #512

Open
HenriBeck opened this issue Aug 23, 2016 · 10 comments
Open

Add support for absolute imports to import/order #512

HenriBeck opened this issue Aug 23, 2016 · 10 comments

Comments

@HenriBeck
Copy link

HenriBeck commented Aug 23, 2016

Currently, when you have the order rule turned on and the option newlines-between set to always
it will give errors when you have something like this:

import a from '/imports/config';
import b from '/imports/some/folder'

This will report having an empty line between the imports which in my opinion isn't correct.

Proposal

Add a new string to the order rule called absolute.

@jfmengels
Copy link
Collaborator

Adding a new import type absolute sounds good to me. I'm not sure why the order rule finds both imports as different, and will look into it.

I'm curious though as to why you import modules this way. Do you mind explaining it to me? :)

@HenriBeck
Copy link
Author

Sure, I'm using meteor and it allows me to import files from the root dir with /.
I'm using the root import for better readability.

For Example:

import a from '../../../client/components/button';

So i have this instead:

import a from '/imports/client/components/button';

So it's clearer where the file is located and it easier to find them.

@jfmengels
Copy link
Collaborator

Hmm, sounds like a resolver would be more appropriate in your case. @benmosher thoughts on that?

@HenriBeck
Copy link
Author

The Resolver isn't the problem, I'm using this resolver: https://github.com/clayne11/eslint-import-resolver-meteor, which handles the absolute imports correctly.

The problem is the order.
https://github.com/benmosher/eslint-plugin-import/blob/master/tests/src/core/importType.js#L58
This would be my case and it returns 'unknown'.

@benmosher
Copy link
Member

@jfmengels yeah, it's a little weird because they aren't technically absolute on the filesystem, but they certainly look that way.

I'm thinking about making it possible for the resolver to specify what importType a given module ought to be, and failing over to the existing path-based one if not provided. Tracking with #479.

Given that, I think these imports would be classified as project type by the resolver, and then it would work without the absolute type.

So the question becomes: do you think it's worth adding the absolute order group type, regardless of if/when resolvers gain the ability to classify import types?

@jfmengels
Copy link
Collaborator

it's a little weird because they aren't technically absolute on the filesystem, but they certainly look that way

My thoughts exactly :)

do you think it's worth adding the absolute order group type, regardless of if/when resolvers gain the ability to classify import types

Pretty sure you can do

import foo from '/home/users/me/foo.js';

in "vanilla" Node, so it's probably worth it, yeah. Would make a rule to forbid absolute imports (which sounds to me like a great rule, hopefully never useful) pretty easy.

I'm thinking about making it possible for the resolver to specify what importType a given module ought to be, and failing over to the existing path-based one if not provided. Tracking with #479.

Given that, I think these imports would be classified as project type by the resolver, and then it would work without the absolute type.

Sounds good to me. And when the resolver doesn't overcharge the importType detection, or returns null or something, it should fallback on the core importType algorithm.

@jfmengels
Copy link
Collaborator

Removing myself from this issue, as it is now more of a resolver thing, which I'll gladly leave to someone else 😅

@benmosher
Copy link
Member

actually @jfmengels, I think this is fixed by the changes in #530? since you added absolute?

@jfmengels
Copy link
Collaborator

Well, the original problem is solved, but you mentioned this which hasn't been done, and is probably nice to have. (maybe rename the issue or create a new one?)

@benmosher
Copy link
Member

benmosher commented Sep 1, 2016

Ah, yeah, I'm tracking that via #479 at the moment. (but it looks like it's not necessary to close this issue, just me getting distracted and off track... 😅)

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

No branches or pull requests

4 participants