-
Notifications
You must be signed in to change notification settings - Fork 25
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
Check tie-breaking with importOrderCaseInsensitive and type imports #23
Comments
While we're at it, we also need to break ties in case two imports only differ by whether they have a type specifier. I could make this a separate issue, but since it's both related to tie-breaking, I'd like to keep it in one issue |
I don't think this is a super big issue if we can count on a stable sort algorithm. If we can't then we have trouble. (Not-to mention that case-sensitivity issues are only possible on some-file-systems or with fake packages in node's resolution hierarchy.) NodeJS's sort is not guaranteed stable, so I recommend we switch to a stable-sort. Here's a random one from npm: https://www.npmjs.com/package/stable / https://github.com/Two-Screen/stable -- it has zero deps, and does have TypeScript. Additionally, I learned that So this proposal adds a dep, in exchange for removing a dep & a hand-written type-definition file. haha |
I learned today that Javascript's language spec now requires Not sure when this made it to NodeJS, but I'm sure that implies we don't even need a dependency at all. That means this proposal allows us to remove the dependency & handwritten type-definition file and just use built-in JS! 🎉 |
Opened a related PR in #28 |
Hi @fbartho, I'm not exactly sure that having a stable sort is what we want in this case. As @blutorange mentioned in the comment in the issue description:
Just because a sort is stable, doesn't mean this will happen, it means that whatever order the rows were in to begin with is how they will stay, so
would remain that way, and
would also not be changed. We want a deterministic output instead, so as said, we have to fall back to case-sensitive. |
I’m arguing that as an opinionated sorting tool, we should consider whether case-sensitivity is something we need to specially support. While technically NodeJS does treat modules as case-sensitive, many/most file systems are case-insensitive (case preserving) by default. An opinionated tool should express its opinion here to meet the majority need. We should avoid writing special handlers for what we can consider a bugged input in the common case. Options: I’m suggesting that option A is our best move, because: being non-deterministic but stable, users who do fall into this edge case can manually handle the problem by moving the imports how they like. Option B is more code and another decision: “which should go first, UPPER or lower variants?” — and this decision begs another config option which I oppose. — also, technically locale has an answer to which goes first #28 (comment) Anyhow, I’ve said my piece on this, happy to get in line with whichever decision you want to make. Let’s just pull the trigger and get the breaking release started. I’ll be much happier once we ship that. |
@fbartho given the clearing up of misunderstanding in #28 (comment), does that change your stance above? |
No, unfortunately, talking things through as I did above still makes me think that option A is the best one. If you’re asking my opinion I still think we should go with Option A, then plan the breaking changes release, and restore #28 with the big breaking release. If you want to make the call the other direction, then we need to implement the case-sensitive locale-based sorting. |
Got it, thanks. @blutorange do you have any thoughts or opinions here? |
Well yeah sorry, I am pretty busy with real life. What's important in my opinion is that the formatter has a deterministic output and does not produce different results on different systems. That said, I'd be fine with both options,. |
@fbartho I think this issue can be closed now, what do you think? |
Indeed, this is addressed by our stripping of various options in the breaking release, and otherwise changing things. I’ll close this ticket, and we can create a new one if something similar is still valid! |
Relevant: but TypeScript/MS have an option now to control case-sensitive sorting https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#case-insensitive-import-sorting-in-editors |
As mentioned in #22 (comment), we should check to be sure we correctly break ties when two imports differ only in capitalization, and
importOrderCaseInsensitive
istrue
. Maybe this is already handled, I haven't checked.The text was updated successfully, but these errors were encountered: