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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to override default position of "3rd party" imports #65

Merged
merged 3 commits into from
Oct 21, 2021
Merged

Add the ability to override default position of "3rd party" imports #65

merged 3 commits into from
Oct 21, 2021

Conversation

risen228
Copy link
Contributor

@risen228 risen228 commented Jul 4, 2021

The problem

I am the author of ESLint Kit.

There, I wanted to migrate from the buggy import/sort ESLint rule to something better and easier.
And I found your library pretty good in this.

But my case is not as simple as the examples in the README - I need to get the following order:

// the nodejs stdlib
import path from 'path'
import fs from 'fs'

// 3rd party modules
import YAML from 'yamljs'
import chalk from 'chalk'

// my aliases
import { foo } from '@app/foo'

// the relative imports
import { bar } from '../../bar'
import { baz } from '../baz'
import { moo } from './moo'

I found that it's very hard to do in the current version of sort-imports plugin.
The reason is that 3rd party modules are always moved to the top.
So, to get this behavior I have to write a really complicated RegExps.
After that, the prettier config may become very hard to extend and support.

The solution

I offer you the special word <3RD_PARTY>.

It can be placed in the importOrder rule, just like the RegExps.

<3RD_PARTY> "collects" the imports that are not matching to any RegEx specified in importOrder.

The bonuses

  • This change is not breaking
  • It's covered with tests
  • I think the algorhythm became a much simpler and faster 馃槉

@risen228
Copy link
Contributor Author

@ayusharma Can you check it please?

@ayusharma
Copy link
Collaborator

Hi @risenforces, Sorry for the delayed response. We are looking into it. 馃憤

@ernusBothma
Copy link

I was about to add a feature request for this.
Eagerly awaiting V3

Copy link
Collaborator

@byara byara left a comment

Choose a reason for hiding this comment

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

Looks really good to me, I left a few comments.
Could you please add another snapshot test case for imports all being third-party?
I also changed the base branch to v3.x so we can release this in the upcoming version.

Comment on lines 51 to 59
const getMatchedGroup = (node: ImportDeclaration) => {
for (const { group, regexp } of regexps) {
const matched = node.source.value.match(regexp) !== null;
if (matched) return group;
}

// nothing matched, move to REST
return THIRD_PARTY_SPECIAL_WORD;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be better if we extract these kinds of functions and abstract them to a separate file + unit tests so they are more maintainable. What do you think?

groups[matchedGroup].push(node);
}

type ImportOrLine = ImportDeclaration | ExpressionStatement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please extract the types outside and import them on top.

@byara byara requested a review from ayusharma October 20, 2021 09:15
@byara byara changed the base branch from master to v3.x October 20, 2021 09:24
@byara
Copy link
Collaborator

byara commented Oct 21, 2021

Hey @risenforces, as we want to release the v3 very soon, I'll take over and apply the changes.

@ayusharma
Copy link
Collaborator

Hi @risenforces , thanks for the PR 鉂わ笍 What do you mean by "REST" terminology in your PR ?

@risen228
Copy link
Contributor Author

Hi @risenforces , thanks for the PR 鉂わ笍 What do you mean by "REST" terminology in your PR ?

I meant this. But as I remember, I replaced these things everywhere.. Did I forgot to do it in some place?

@ayusharma
Copy link
Collaborator

Hi @risenforces, There were some breaking changes from the other PR. I took some freedom to modify some parts of the code and make it mergeable. I hope you would not mind my interfering. Thank you so much for the amazing contribution. 馃帀

@byara byara merged commit 6c9bb36 into trivago:v3.x Oct 21, 2021
Version 3: New features on Babel compiler automation moved this from In progress to Done Oct 21, 2021
@ayusharma
Copy link
Collaborator

Hi @risenforces , Thank you so much for your PR. Please respond to the notification here. #118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants