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

hyphens and slashes ordered differently than with dprint #127

Open
mtqperson opened this issue Jan 31, 2023 · 12 comments
Open

hyphens and slashes ordered differently than with dprint #127

mtqperson opened this issue Jan 31, 2023 · 12 comments

Comments

@mtqperson
Copy link

If we have one import foo/bar and another foo-bar, this (excellent) plugin and dprint seem to have different opinions about the correct order of those.
(In my case it was { App } from 'app/app' and { appInit } from 'app-init').

I'm not saying that either is right, but the fix was a bit hard to find because the option isn't documented in dprint's TypeScript plugin.
My solution was set "module.sortImportDeclarations": "maintain" in dprint.json.

Maybe a note in the README could be helpful, otherwise please feel free to close this as it can still be found by searching. And thank you for the nice plugin.

@mtqperson mtqperson changed the title hyphens and slashes ordered differently than dprint hyphens and slashes ordered differently than with dprint Jan 31, 2023
@lydell
Copy link
Owner

lydell commented Jan 31, 2023

Hi!

Previous issue about this, in case you are curious: #87 and #91

module.sortImportDeclarations is documented here: https://dprint.dev/plugins/typescript/config/

I’m just trying to understand your case. Is it like this?

  1. You use dprint.
  2. dprint sorts imports by default, but does not group them – it preserves the grouping in the input, and only sorts within groups.
  3. You want automated grouping so you use this plugin as well.
  4. You ran into the difference in - vs / sorting.
  5. You found that sorting can be turned off in dprint.

If that’s correct, then yes, I think it could make sense to mention this in the readme!

@mtqperson
Copy link
Author

Hi!

Thank you for your response & sorry for the dupe, I just searched the issue tracker for "dprint". 🙂

And you're right that it's perfectly documented, I somehow didn't find that but used GitHub code search to discover it...

Yes, that's mostly correct, except the order:
We used eslint-plugin-import, but it had obscure bugs related to monorepos and multiple tsconfigs, and it became clear that making ESLint actually understand imports was slower and more complex to configure than it's worth.
So we've used simple-import-sort for a while, and only recently are trying to replace Prettier with dprint. Didn't expect it to touch our imports and make ESLint angry.

In retrospect I'm not sure why it was so hard to find the option in dprint's documentation. 😅

lydell added a commit that referenced this issue Jan 31, 2023
@lydell lydell mentioned this issue Jan 31, 2023
@lydell
Copy link
Owner

lydell commented Jan 31, 2023

@jakebailey Is this a problem for your “sorting consistency between TypeScript, dprint and eslint-plugin-simple-import-sort” goal? (#124) That the tools might disagree about the order of these two:

import { App } from 'app/app';
import { appInit } from 'app-init';

@jakebailey
Copy link

Testing TS 5.0 with:

    "typescript.unstable": {
        "organizeImportsCollation": "unicode",
        "organizeImportsCaseFirst": "upper",
        "organizeImportsIgnoreCase": false,
        "organizeImportsNumericCollation": true
    },

Both dprint and TS's "sort imports" sort the above example like:

import { appInit } from 'app-init';
import { App } from 'app/app';

But this plugin sorts it like:

import { App } from 'app/app';
import { appInit } from 'app-init';

I'm a little confused because the above settings in TS 5.0 should match the plugin because both use Intl.Collator; is is this potentially a bug in the plugin?

@jakebailey
Copy link

jakebailey commented Jan 31, 2023

Of course, the best setting for dprint when used with the plugin is probably to set it to maintain on all of the import/export related settings, which makes it match prettier (which won't move them at all). Though I do hope to be able to bulk-reformat using dprint and not have to run things back and forth between eslint and dprint to reach the steady state, so maybe it is a problem.

So, from my perspective, the PR that adds the note is fine, but I am a little more concerned about TS and the plugin not agreeing for some reason (since I'm trying my best to get alignment so stuff like auto-imports agrees with the desired ordering). Thankfully this is probably a really uncommon case, unlike type imports.

@jakebailey
Copy link

jakebailey commented Jan 31, 2023

Just for reference, this is effectively what TS is setting with the above config:

["app/app", "app-init"].sort(new Intl.Collator("en", {
	sensitivity: "base",
	caseFirst: "upper",
	numeric: true,
}).compare)

Which my console says is ['app-init', 'app/app']. The only difference between what's documented in the readme of this repo is that I'm setting caseFirst to "upper", versus instead falling back to an ordinal comparison, but that shouldn't matter in this example because they aren't equal.

So the fact that the plugin is returning the opposite is curious.

@lydell
Copy link
Owner

lydell commented Jan 31, 2023

TypeScript is doing the reasonable thing; this plugin is the odd one out. I implemented the “../ sorting“ (the more ../, the earlier the import sorts) in a very … “clever” … way where I swap some characters (including - and /) and then just do a regular sort on it. #87 (comment)

Last time this was reported, my answer was that it doesn’t matter what order punctuation is sorted in as long as it was stable. But now there’s seems to be a reason to change.

@toerndev
Copy link

toerndev commented Feb 1, 2023

@lydell FWIW I think your way with special handling of '/' made more sense - the slash obviously determines path topology, if we merely sort it according to the ASCII table then some special characters might come before that and some after, essentially interleaving imports from different paths.
Intl.Collator as configured above gives "appapp", "app/app", "app-app", i.e. entering a subpath and then exiting it again.

But yeah not going against TS might be more pragmatic, ultimately.

@jakebailey
Copy link

jakebailey commented Feb 1, 2023

Yeah, it's tricky; I think I'd actually expect them to be sorted by first splitting on slashes, then sorting them in pieces, but that's not what this new (still unstable) TS feature does, as well as dprint (probably because such a thing is slow). Thankfully I think this sort of thing is rare.

I'm planning on investing some time into checking all of these edge cases soon; I don't want to accidentally make the plugin change again only to be wrong and have to do another release, so feel free to leave the behavior and suggest disabling dprint instead.

@lydell
Copy link
Owner

lydell commented Feb 1, 2023

Is there a way I can play around with the new sorting in TypeScript?

I also noticed that this is sorted according to Intl.Collator:

import a from "./a";
import React from "react";

But dprint sorts it as:

import React from "react";
import a from "./a";

Lots of nuance here, it seems.

@jakebailey
Copy link

Sure, you can set this in your user or workspace config: https://github.com/microsoft/TypeScript/pull/52090/files#diff-3bf027c5abb5653676adae4eb5c21ffc57cbf3b883b7b4ca691f356fde61e792

And then install the TS nightly extension in VS Code and ensure that the selected version is some version of 5.0 (there's a command palette command for selecting the version).

I've just been checking out that PR and making a random file in the compiler folder and testing there.

@jakebailey
Copy link

Feedback is welcomed; this isn't fully polished yet.

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

No branches or pull requests

4 participants