-
Notifications
You must be signed in to change notification settings - Fork 72
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
Group for first party packages #18
Comments
Hi! Like mentioned in #4 (comment), I’d recommend choosing an alias that does not look like an npm package name. I think that is less confusing as well. Would that work for you? |
Not really, no. Our project is very big and has a lot of people on it and I only recently managed to convince everyone that using the |
Thanks for your reply. I understand that it would be inconvenient to make another such big change and have to re-teach everyone to use for example On the other hand – if we did add an option for this you would still end up with 400+ files where the sorting changed? I really want this plugin to stay free of options for as long as possible, because as soon as we add one it feels like opening the flood gates to more. And I’m not sure trying to parse webpack/Rollup/TypeScript/whatever configs is going to be sustainable. ¯\_(ツ)_/¯ |
There's a bit of a difference between me adding an additional rule to the automatic I entirely appreciate you not wanting to add more complexity to this plugin, and if you don't want to add it I will probably end up writing my own plugin. 🙂 FWIW I think parsing any kind of config is way overkill. A |
Hmm … could you add a custom little ESLint rule that autofixes from If you do write your own plugin then feel free to share it here! |
Well, admittedly using Ideally I'd want a plugin that just alphabetises imports by block and then let a different one (eg import/order) make the actual blocks, but I guess with the unknown order that eslint will apply rules in, that wouldn't be very stable. |
Allow custom prefixes, this is very helpful for developers, hope to support this feature. |
Normally I use Would be amazing if settings: {
'import/resolver': {
'babel-module': {
root: ['./src/'],
extensions: ['.js', '.jsx', '.ts', '.tsx', '.d.ts'],
},
}, |
@heygrady Don’t you think it’s confusing that some npm-package-looking imports actually resolve to local code? What’s your thought on my recommendation so far to use for example |
Use the resolve.alias feature of webpack is very useful, otherwise you might import to some common code in the project like this: import xx from '../../../utils/xx' Actually, because of the webpack support import xx from '@utils/xx'
Suggest expose the configuration to allow developers to modify. |
Personally, I’ve always bit the bullet and used those super ugly IMO,
Sorry, that’s not what I meant. What I meant is – if I say "The solution is to use a non-ambiguous prefix such as |
I think the issue is that it's a very common practice. For instance, absolute imports are supported by create-react-app. There are some caveats with absolute imports (like name collisions with an existing npm package). However, it's not really valuable to bike-shed what people should be doing. The important bit here is that people are using absolute imports. It's supported by eslint, babel, typescript, rollup and webpack. There is a non-zero set of people who use eslint and babel, use absolute imports for perfectly valid reasons, and who would also like to have their imports sorted. It's worth noting that your plugin appears to do the "right thing" with import { foo } from 'absolute' // <-- in the wrong spot
import { bar } from 'npm-package'
import { baz } from 'absoluteCamelCase' // <-- in the right spot
import { foo } from './relative' It's also worth noting that |
Those are very good points. |
Keep in mind - absolte imports are 100% "native" for TypeScript. |
@theKashey Interesting! Can you expand on that? Is there a link to some documentation? |
|
Grouping idea:
So, instead of using validate-npm-package-name to decide what should go in group 2, we use a list of Node.js core modules and look in the closest package.json. Do you think it would work? EDIT: Hmm, probably not. I have actually aliased "react" to "preact" in a project, which means that "preact" is present in package.json while "react" isn’t. Maybe we could look for directory names inside |
Would not for the |
Ah, good point. Looking inside I won’t add |
"Well known modules"?
|
“Well known modules” won’t solve the problem. Users can alias anything. “Simple modules” unfortunately won’t work either. Some npm packages let you import from Keep the ideas coming! Some rough ideas I have:
|
What if we added a function option like this: // .eslintrc.js
module.exports = {
// ...
rules: {
"simple-import-sort/sort": ["error", {
transform: (from, filepath) => {
return from;
},
}],
}
};
This way you could for example inspect the passed So. If this existed – would it be useful to you? Would it be too much work implementing such a function? Feel free to post your |
Another idea is to add support for tsconfig.json and jsconfig.json, like create-react-app. If you use TypeScript you probably already have a tsconfig.json. If you use JavaScript you’ll have to create a jsconfig.json if you use non-standard stuff, which is probably a good idea anyway. |
Or maybe I’m just over thinking this, and we should just add an option that is an array of package names, like @Lexicality suggested in the first place. |
I’ve made up my mind. TypeScript is a big thing these days. If you use “absolute imports” with TypeScript, you have to set it up in tsconfig.json, right? Otherwise TypeScript won’t understand your imports and give errors. So it would be really nice if eslint-plugin-simple-import-sort Just Works with TypeScript out of the box. What about if you don’t use TypeScript? Well, the easiest thing for the plugin is to go with the jsconfig.json approach. Same thing, just another file name. As a bonus your imports will then work in VSCode as well. And there’s nothing stopping other editors and tools from reading jsconfig.json either. create-react-app already supports it, as mentioned. Pros:
Cons:
This is the plan. I think that we only need to look at I’m thinking that the logic will be something like this:
Here’s a good explanation on how baseUrl and paths really work: |
No objections.
|
Yeah, I’m currently researching if there are any packages already that do this. Maybe this one: https://github.com/dividab/tsconfig-paths |
Status update: Last time I looked at this, I couldn’t find any module that supports tsconfig.json and jsconfig.json. But maybe we don’t need to support jsconfig.json at all, because tsconfig.json might work for JS too: See prettier/prettier#6702 (comment) and onwards. |
@lydell Something good to now: I'm using absolute imports in typescript but I don't use the Why not using #18 (comment) and allow aliasing certain modules in the config for this plugin? In the past I've used https://github.com/galooshi/import-js and I found that to be really good 😄 |
@entropitor Thanks for letting me know about |
I have a thing coming up that will allow customizing this. I’m still interested in supporting TypeScript out of the box, but I’ll open a new issue for that. |
Hi, sorry for not taking part in the discussion, I've been extremely busy lately. {
"allowJs": true,
"baseUrl": "media",
"paths": {
"*": ["./*", "./web/lib/*"]
},
} With a file structure a bit like this:
And example-file.ts looking a bit like this: import React from "react";
import lib1 from "vendored-lib1";
import lib2 from "vendored-lib2";
import { Foo } from "web/components/foo";
import aaa from "web/modules/aaa";
import { Bar } from "./bar"; I'm not completely against our vendored libs becoming 1st party packages - but I'd sort of prefer them not to if possible. Sorry for being unhelpful 😬 |
You’ll be able to put your vendored libs anywhere you want with my upcoming thing. |
v5.0.0 has now been released with the Example: {
"rules": {
"simple-import-sort/sort": [
"error",
{
"groups": [
["^\\u0000"],
["^@?\\w"],
["^(web|vendored-lib)(/.*|$)"],
["^\\."]
]
}
]
}
} The automatic tsconfig.json idea is tracked in: #31 |
We have a module alias called
web
for our local code (like@
in #4) and I'd like it to be sorted into it's own group like this:import/order
supports this by doing a bunch of magic with Webpack parsing, but just having a list of first party packages as a config variable would be a lot easier.Thanks
The text was updated successfully, but these errors were encountered: