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

<TYPES> & <BUILTIN_MODULES> #141

Closed
1 task done
cevr opened this issue Jan 29, 2024 · 11 comments · Fixed by #155
Closed
1 task done

<TYPES> & <BUILTIN_MODULES> #141

cevr opened this issue Jan 29, 2024 · 11 comments · Fixed by #155
Assignees

Comments

@cevr
Copy link

cevr commented Jan 29, 2024

Your Environment

  • Prettier version: 3.x.x
  • node version: 20.x
  • package manager: [pnpm@8]
  • IDE: [VScode]

Describe the bug

Currently there is a way to delineate between sorting local type imports, and third party type imports. However, there is no way to delineate between third party and builtin type imports which breaks the sort order if you have those specificed.

To Reproduce

importOrder: [
    "<BUILTIN_MODULES>",
    "",
    "<TYPES>",
    "<THIRD_PARTY_MODULES>",
    "",
    "^~/(.*)$",
    "",
    "<TYPES>^[.]",
    "^[.]",
  ]

Using a importOrder as one above, if you import a type from a builtin module, since there is no way to specify something like <TYPES><BUILTIN_MODULES>, it breaks the sort order.

Expected behavior

<TYPES><BUILTIN_MODULES> should be possible.

Contribute to @ianvs/prettier-plugin-sort-imports

  • I'm willing to fix this bug 🥇
@IanVS
Copy link
Owner

IanVS commented Jan 29, 2024

Thanks for the issue. Could you provide a concrete example we can use as a test case? i.e. Your config, the code before the sort, the result you want, and the result you're getting?

@cevr
Copy link
Author

cevr commented Jan 29, 2024

Thanks for the speedy response!

Given this config

importOrder: [
    "<TYPES><BUILTIN_MODULES>",
	"<BUILTIN_MODULES>",
    "",
    "<TYPES>",
    "<THIRD_PARTY_MODULES>",
    "",
    "<TYPES>^[.]",
    "^[.]",
  ]

I would expect the following imports:

import path from "node:path";

import type fs from "node:fs";
import type { RunResult } from "better-sqlite3";
import { Do, Option, Result } from "ftld";

import { DomainError } from "../lib/domain-error";
import { msTimestamp } from "./utils";

to be formatted as such:

import type fs from "node:fs";
import path from "node:path";

import type { RunResult } from "better-sqlite3";
import { Do, Option, Result } from "ftld";

import { DomainError } from "../lib/domain-error";
import { msTimestamp } from "./utils";

but it is instead formatted as such:

import path from "node:path";

import type { RunResult } from "better-sqlite3";
import type fs from "node:fs";
import { Do, Option, Result } from "ftld";

import { DomainError } from "../lib/domain-error";
import { msTimestamp } from "./utils";

@cevr
Copy link
Author

cevr commented Jan 29, 2024

But also, I suppose an alternative solution for my purposes would be to expose importOrderCombineTypeAndValueImports as an option that I can turn off explicitly. I prefer type imports to be separate to non type imports, as it there are semantic differences to how typescript strips types based on whether it is inline: import { type x } from 'x' vs top level import type { x } from 'x'

@fbartho
Copy link
Collaborator

fbartho commented Jan 29, 2024

@cevr what you demonstrated in the test case I consider a bug in our current implementation — but the more I look at it, the more I realize why it happens, based on our implementation. For complicated parsing reasons, the TYPES pattern happens to have higher priority than other patterns (it happens in a special, complicated, last-pass), BUILTINS on the other hand is a “normal” pattern that just operates on the module-source.

This may be fixable, I’m not sure, but I expect it to be a challenge! — definitely thank you for filing the ticket!


I suppose an alternative solution for my purposes would be to expose importOrderCombineTypeAndValueImports as an option

We don’t want to expose more options for configuration than we have, because it’s both against the “opinionated formatter” ethos of prettier, and because it significantly complicates the implementation, test cases, documentation, and plugin usability overall, and indeed in recent major versions we pared down the options provided by this plugin. I hope this is convincing!

@fbartho
Copy link
Collaborator

fbartho commented Jan 29, 2024

@cevr if the TYPES special word is present as a configuration pattern, then import type expressions are plucked out of other groups, and moved into the types location.

I believe that code partially happens here:

// If type ordering is specified explicitly, we need to break apart type and value specifiers
if (
importOrder.some((group) => group.includes(TYPES_SPECIAL_WORD))
) {
nodes = explodeTypeAndValueSpecifiers(nodes);
}

@fbartho
Copy link
Collaborator

fbartho commented Jan 29, 2024

When presented with

import type fs from 'node:fs';

The code has to make a choice: is this a TYPES import? Or is this a BUILTINS import?

Currently the code says “if you use the TYPES pattern, then you want all types to be together, no matter what”.

return node.importKind === 'type' && includesTypesSpecialWord
? TYPES_SPECIAL_WORD
: THIRD_PARTY_MODULES_SPECIAL_WORD;

I’m less convinced this is an obvious bug now, and rather think it’s an unexpected consequence of an implicit ranking that the TYPES pattern is more powerful/sticky than the BUILTINS pattern.

I could be convinced that the BUILTINS pattern should instead win, though that would be a breaking change in a feature (<TYPES>) that I personally don’t want to use.

Additionally this code is complicated by the fact that types can be a prefix on another pattern, <TYPES>@my-modules/* and that could make for some really complex unintuitive interactions if we want to change how it behaves.

@IanVS
Copy link
Owner

IanVS commented Jan 29, 2024

I need to give it some more thought, and it's been a while since I looked at the code so I can't promise it can be done, but I wouldn't be entirely opposed to adding a <TYPES_BUILTINS> keyword for this situation.

@cevr
Copy link
Author

cevr commented Feb 1, 2024

I can't comment on the best solution, though I would argue that breaking types from non types is always better since typescript will remove type import statements completely, but not inline type specifiers. If the goal is to be opinionated and simple, perhaps that could be a simple solution.

Otherwise, adding another keyword would work! Though I would say it might make sense to make a keyword for third party modules as well for consistency.

Thank you for the responses!

@IanVS
Copy link
Owner

IanVS commented Mar 11, 2024

@cevr I gave this a little more thought, and I think that I found a solution. You should be able to add <TYPES>^(node:) in order to sort the built-in types above the built in modules, so long as you are consistent about using the node: import syntax, which can in turn be enforced using https://github.com/sindresorhus/eslint-plugin-unicorn/blob/702d51bed176a9c2c93bc4a2ca52e700dd0c2339/docs/rules/prefer-node-protocol.md.

Here's an example showing the syntax above in action: e414d30#diff-972a783a7dfae730fe457ea5acd569db03093756f54f1bf794ea521be431f57fR5

Could this be a good-enough solution for you, @cevr? I can also add an example to the README in case this is something others are interested in doing, since it's not immediately obvious it can be done.

@cevr
Copy link
Author

cevr commented Mar 11, 2024

@IanVS Thank you! That's a perfectl solution. Would this work for built ins of other platforms like bun?

@IanVS
Copy link
Owner

IanVS commented Mar 11, 2024

Sure, I don't see why not, you just need some kind of regex pattern to match off of.

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

Successfully merging a pull request may close this issue.

3 participants