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

Line break removed when importOrderCombineTypeAndValueImports is used #54

Closed
1 task
andidev opened this issue Mar 20, 2023 · 20 comments · Fixed by #71
Closed
1 task

Line break removed when importOrderCombineTypeAndValueImports is used #54

andidev opened this issue Mar 20, 2023 · 20 comments · Fixed by #71
Assignees

Comments

@andidev
Copy link

andidev commented Mar 20, 2023

Your Environment

  • Prettier version: 2.7.1
  • node version: v16.17.0
  • package manager: npm@9.3.1
  • IDE: Webstorm

Describe the bug
Sorting imports with option importOrderCombineTypeAndValueImports: true removes a line break at top of prettier anotation if the type import is defined above import line it is merged with.

image

becomes:
image

Expected behavior
Expected a line break like this:
image

Configuration File (cat .prettierrc, prettier.config.js, .prettier.js)

/**
 * @prettier
 */
module.exports = {
    printWidth: 200,
    tabWidth: 4,
    useTabs: false,
    semi: true,
    singleQuote: true,
    jsxSingleQuote: true,
    trailingComma: 'es5',
    arrowParens: 'avoid',
    requirePragma: true,
    bracketSpacing: true,
    quoteProps: 'consistent',
    parser: 'typescript',
    overrides: [
        {
            files: 'src/i18n/*.js',
            options: {
                printWidth: 20,
            },
        },
        {
            files: 'src/i18n/translate.js',
            options: {
                printWidth: 200,
            },
        },
    ],
    importOrder: ['<THIRD_PARTY_MODULES>', '^rootpath$', '^(scripts/.*)$', '(^src/.*)$', '^(test/.*)$'],
    importOrderSeparation: false,
    importOrderSortSpecifiers: true,
    importOrderBuiltinModulesToTop: true,
    importOrderMergeDuplicateImports: true,
    importOrderCombineTypeAndValueImports: true,
};

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

  • I'm willing to fix this bug 🥇
@andidev
Copy link
Author

andidev commented Mar 24, 2023

Also noticed that this plugin (compared to original) does not remove new lines after the prettier pragma comment.
image
Like these line breaks are kept.
Not sure if this is intentional and in case this may help solve this issue by doing a last formatting at end with the first import.

@IanVS
Copy link
Owner

IanVS commented Mar 24, 2023

Thanks, I'll look into this as soon as I get some spare time, but it might be a little while.

@fbartho
Copy link
Collaborator

fbartho commented May 11, 2023

@andidev I don't believe this plugin should remove (all) new-lines after the pragma-comment -- even though the original plugin does so. -- You wouldn't want us to move that comment away from top-of-file if your currently-first import gets sorted lower down.

That said in #71 I've made significant changes to our handling of comments, so once that gets reviewed (and fixed if I made a mistake) your original bug report should no-longer be happening!

@andidev
Copy link
Author

andidev commented May 11, 2023

well the pragma comment should of course not be sorted lower. however removing the blank string at top if any would not harm according to me but I get this might be a taste what you like or not.
however the main thing with current version is that it adds a black line sometimes when sorting.
if 71 solves that I am more than happy!

@IanVS
Copy link
Owner

IanVS commented May 12, 2023

Hi @andidev! We've just released 4.0.0-alpha.4 which includes changes to the way we handle comments. We have a migration guide, but mostly we are becoming more opinionated on options to make the plugin easier to configure.

We'd love if you were able to try it out and let us know what you think!

@andidev
Copy link
Author

andidev commented May 13, 2023

Wow nice work, like the changes!

However I cannot test this issue since
importOrderTypeScriptVersion: '4.8.2',
does not seem to work. Setting it I would seem to trigger the importOrderCombineTypeAndValueImports behaviour from old versions. @IanVS @fbartho

Otherwise I tested it on our flow and typescript largest code bases and besides that it seem to work (however the imports where already sorted so not sure how much test branches this validates)

@IanVS
Copy link
Owner

IanVS commented May 13, 2023

@andidev yes, by default we will now combine type and value imports for versions of typescript that support that syntax. If you'd rather keep type imports separate, and sorted to their own section, you can use the <TYPES> keyword in your importOrder. You can also just not provide a typescript version, and the type and value imports will not be combined. That is the only usage of that version setting at the moment.

@andidev
Copy link
Author

andidev commented May 13, 2023

@IanVS I think you misunderstood me. I know it is suppose to combine it by default but my problem is it does not combine it. Types and non types importer from same file is kept on separate rows.
And I have specified the correct ts version according to the migration guide.

@fbartho
Copy link
Collaborator

fbartho commented May 13, 2023

@andidev We have a test-case that verifies that we combine them here:

import type {Foo} from './foo';
import {fooValue} from './foo';
import def from './bar';
import type {Bar} from './bar';
import * as namespace from './baz'
import type {Baz} from './bar';
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
import def, { type Bar, type Baz } from "./bar";
import * as namespace from "./baz";
import { fooValue, type Foo } from "./foo";

If you want us to debug it further, I'd be happy to help -- please supply your sample code! (Ideally as a new ticket, with your prettier.config settings too)

@IanVS
Copy link
Owner

IanVS commented May 13, 2023

Oh okay, thanks for explaining! A new issue would be great if you don't mind. Also note, it only works when you use import type, since we don't inspect the actual import itself to find out whether it's a value or import in the file where it is being exported.

@andidev
Copy link
Author

andidev commented May 13, 2023

So it looks like it my error was a webstorm doing the sorting instead of prettier (even though prettier is configured).

@IanVS
But this issue is still not solved. I can reproduce it with following code:

/**
 * @prettier
 */
import type { FooValue } from './foo';
import { fooValue } from './foo';

formats to:

/**
 * @prettier
 */

import { fooValue, type FooValue } from './foo';

@fbartho
I even found another case that is sorting it even worse (think its connected to this issue since probably the same implementation causing it):

/**
 * @prettier
 */
import { requireValue } from 'src/utils/require/requireValue';
import type { FooValue } from './foo';
import { fooValue } from './foo';

formats to:

import { fooValue, type FooValue } from './foo';
/**
 * @prettier
 */
import { requireValue } from 'src/utils/require/requireValue';

@IanVS
Copy link
Owner

IanVS commented May 13, 2023

Currently, we require one blank line after comments at the top of the page in order to "detach" them from the following import statement. If you have a comment directly above an import, we treat it as a leading comment of that import, and it will move with the import if it gets sorted.

Personally, I prefer having a blank line separating comments, pragma, or directives at the top of the page, but I realize not everyone might. Unfortunately in this case it's just something that we need to be opinionated about so that we can have predictable behavior.

I believe the issue you first reported, about imports moving into the same line as the end of a comment block, is solved now, right?

@andidev
Copy link
Author

andidev commented May 13, 2023

@IanVS its not solved. Its the first example I mentioned in my comment is this issue.
new line or now new line after prettier import does not matter I think, but adding a new line in some case just because two imports are moved does not make sense.
Also as in the comment there seem to be some sorting issue causing an import to be moved above the prettier comment which breaks the prettier formatting of file. This one I would say is critical with the new release, just tested and this does not happen in old version.

@IanVS
Copy link
Owner

IanVS commented May 14, 2023

/**
 * @prettier
 */
import type { FooValue } from './foo';
import { fooValue } from './foo';

Reformatting to

/**
 * @prettier
 */

import { fooValue, type FooValue } from './foo';

Is not a bug, that's desired behavior.

I can see why the second example you gave could be a problem, though. I think we can find a way to deal with that case.

@andidev
Copy link
Author

andidev commented May 14, 2023

Ok, then it's just a strange behavior for a code formatter then. Because it formats a new line in case an import is merged to another if it's after the top comment. But in case the import sorted there is not merged with another import line it does not format a space.

We formatted our whole code base and that is where I saw we sometimes got a new line after prettier comment and sometimes not. Just seemed strange. But it's an minor esthetic issue or maybe I am missing something so not gonna argue for it to be honest.

Either way I appreciate the work on this import sorter!
Thanks! It helps us a lot reducing and simplifying import related merge conflicts.

@IanVS
Copy link
Owner

IanVS commented May 14, 2023

Thanks for your input, and I'm glad this is useful to you. We're going to try making a change so that comments above the first import never move, and I'll see what we can do about the addition of that extra newline as well.

IanVS added a commit that referenced this issue May 16, 2023
Fixes #81 

- This also reverts my `alpha.4` behavior-change where top-of-file
comments always got a newline injected below them. Now we neither inject
nor delete 1 blank line below the last top-of-file-comment.
- This reverts snapshot changes (around top-of-file blank-lines) as
compared to our pre-4.0.0-alpha.4 behavior.
- Fixes some cases where we would introduce a newline when combining type and value imports (reference #54)

---------

Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
@IanVS
Copy link
Owner

IanVS commented May 16, 2023

@andidev would you mind trying 4.0.0-alpha.5? We made some changes to better preserve comments at the tops of files.

@andidev
Copy link
Author

andidev commented May 16, 2023

@IanVS
lgtm, only thing I noticed was this case adds a new line

/**
 * @prettier
 */
import { requireValue } from 'src/utils/require/requireValue';
import type { FooValue } from './foo';
import { fooValue } from './foo';

formats to

/**
 * @prettier
 */

import { fooValue, type FooValue } from './foo';
import { requireValue } from 'src/utils/require/requireValue';

but original issue seems to be solved actually and also formatting

/**
 * @prettier
 */
import type { FooValue } from './foo';
import { fooValue } from './foo';

mentioned earlier formats correctly now without a line break.

fbartho pushed a commit that referenced this issue May 16, 2023
This should address
#54 (comment).

Note, we don't currently have a way to _force_ or _remove_ a line at the
top, under top-of-file comments, but I think this is okay. We preserve
whatever the user had before, which seems fine. We can re-evaluate in
the future if we hear from users.
@IanVS
Copy link
Owner

IanVS commented May 16, 2023

Thanks, we had a bug, https://github.com/IanVS/prettier-plugin-sort-imports/releases/tag/v4.0.0-alpha.6 should fix that for you now. Sorry for the back-and-forth, but thanks for testing out the alphas!

@andidev
Copy link
Author

andidev commented May 27, 2023

nice just upgraded and tested it and cannot find any case yet causing empty lines or removed ones.
Great work!

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