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

Improve default importOrder #76

Closed
IanVS opened this issue May 12, 2023 · 3 comments · Fixed by #83
Closed

Improve default importOrder #76

IanVS opened this issue May 12, 2023 · 3 comments · Fixed by #83
Assignees

Comments

@IanVS
Copy link
Owner

IanVS commented May 12, 2023

Is your feature request related to a problem?
Currently, the default importOrder that we ship is kind of worthless, and only sorts everything alphabeteically.

Describe the solution you'd like
As discussed in #22, let's make the default order be:

importOrder: [
	// builtins (already happens no matter what the `importOrder` is set to)
	'<THIRD_PARTY_MODULES>',
	'^[./]', // Relative (local) imports
]

Describe alternatives you've considered

We could get fancier, but I think this is a sane default that most users will benefit from, and they're able to customize it further if they want.

@IanVS IanVS self-assigned this May 12, 2023
@fbartho
Copy link
Collaborator

fbartho commented May 14, 2023

Right now, our default is [] -- but, we automatically inject built-ins to all queries (not-user-controllable), and then we automatically inject THIRD_PARTY_MODULES if it's not present somewhere in the list.

So the effective default is already as you described:

[
	// builtins (already happens no matter what the `importOrder` is set to)
	'<THIRD_PARTY_MODULES>'
	// Implicit "everything else"
]

I think that means this ticket is moot!


I think someone could convince me that we need to let users write all the regexes they want, to do something like:

[
  '<BUILT_INS>', // should we add special word?
  '@*', // Namespaced packages regex
  '^[^.]', // Non-local-imports
  '^[.], // All local imports
]
// Notice how there's no '<THIRD_PARTY_MODULES>' anywhere?

@fbartho
Copy link
Collaborator

fbartho commented May 14, 2023

I think we forgot to discuss the impact of removing importOrderSeparation on #76

I think our default sort-order should be:

[
  built-ins, // 1
  "", // 2
  "<THIRD_PARTY_MODULES>", // 3
  "", // 4
  "^[^.]", // 5  -- absolute imports
  "", // 6 -- Should we have a gap?
  "^[.]", // 7 -- relative imports 
]

What do you think? should I remove all the gaps? just no-gap in step 6?

We could simplify our defaults by deleting 5-7, and having the new step 5 be ".*" // everything else but we run the risk that absolute imports are alpha sorted below relative imports. "./this/is/relative" < "apps/absolute/path" === true

@IanVS
Copy link
Owner Author

IanVS commented May 15, 2023

The previous default was not to add spaces between imports, and users had to opt in with importOrderSeparation. I think we should keep that same general idea, whereby the default is not to add spaces between import groups, and only sort them. Users can add explicit gaps where they want by specifying them in their importOrder, and we can give some examples of common patterns in the README, which users can copy and use.

Currently, if no importOrder is provided, this is what I get:

exports[`defaults.js - babel-verify > defaults.js 1`] = `
import c from "./c"
import b from  "../b"
import a from "../../a"
import foo from 'foo';
import bar from 'bar';
import fs from 'fs';
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
import fs from "fs";
import a from "../../a";
import b from "../b";
import c from "./c";
import bar from "bar";
import foo from "foo";

`;

Note that the "third party" imports are placed after relative imports. I think we should prevent that, by using:

[
  // built-ins (We can make this a special word in the future if people ask for it, but I don't think we have to right now),
  "<THIRD_PARTY_MODULES>",
  "^[.]", // relative (local) imports
]

@IanVS IanVS closed this as completed in #83 May 16, 2023
IanVS added a commit that referenced this issue May 16, 2023
Fixes #76 

```js
[
            // built-ins are implicitly first
    '<THIRD_PARTY_MODULES>', // Actually means "all things that aren't otherwise matched" aka THIRD_PARTY_IMPORTS & Absolute Path Imports
    '^[.]', // relative imports
],
```

- There's no way to separate THIRD_PARTY_MODULES and absolute-imports
currently
- We can discuss a renaming for THIRD_PARTY_MODULES later

---------

Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants