Imports may get removed if using Prettier range-start/range-end options about prettier-plugin-organize-imports HOT 22 CLOSED

@alexander-doroshko fixed in 3.2.4, it now completely skips when formatting a range, which is based on the following conditions:

• either options.originalText is truthy
• or options.rangeStart !== 0
• or options.rangeEnd !== Infinity and options.rangeEnd !== code.length

from prettier-plugin-organize-imports.

Cool! Thanks a lot @simonhaenisch, @MattLish, and @MattLishmanYara :)

from prettier-plugin-organize-imports.

@simonhaenisch Would it be possible to just have this plugin do nothing when a range is passed?
While the problem shows itself in certain IDEs, I would say this is a bug in the plugin to be destructive when using prettier to format a range of a file.

from prettier-plugin-organize-imports.

Close/open the project in the IDE to make sure there are no caching issues

@alexander-doroshko Ah, that was the issue 😅
After closing and reopening, I can confirm this PR fixes the issue experienced in the IDE 👍

@simonhaenisch Looks like the PR is sufficient, I would appreciate it if you could review it when you have time please 😄 .

from prettier-plugin-organize-imports.

I'm gonna try to find some time to double-check your PR because if code contains the full file, then the plugin should do the right thing (not remove imports) even when a range is set (since the plugin just passes whatever code to the TS API).

from prettier-plugin-organize-imports.

Very interesting, thanks for sharing. Not sure what can be done about this implementation-wise though, since by passing only a range to Prettier you are taking away the whole context of the file which obviously is needed for the TypeScript API to figure out which imports are or aren't used.

In this use-case you'll just have to turn on the skip destructive code actions option (preventing unused imports from getting removed).

Or request an option in those IDE's Prettier integrations to not do a partial format.

from prettier-plugin-organize-imports.

@simonhaenisch Thanks for the answer.

Is it really not possible for the prettier-plugin-organize-imports plugin to get the full context of a file when formatting a range?

If so, I'd also be happy if the plugin does nothing.

from prettier-plugin-organize-imports.

Is it really not possible for the prettier-plugin-organize-imports plugin to get the full context of a file when formatting a range?

Yeah sure could probably manually read the file but then you might as well just tell your IDE to format the whole document not just the imports? 🤷🏻‍♂️ anyway seems like a bad thing to implement (seems fragile, creates performance overhead and maintenance cost) when Prettier already gives you the code it's supposed to format.

If so, I'd also be happy if the plugin does nothing.

Then either enable the option I mentioned or remove the plugin?

from prettier-plugin-organize-imports.

Or do you mean the plugin should only do nothing if a range is provided? Could make sense

from prettier-plugin-organize-imports.

Or do you mean the plugin should only do nothing if a range is provided? Could make sense

Yes, that's what I mean.

I'm from JetBrains, so I'm looking at the issue from the IDE's point of view. IDE can't remove the user's plugin or reconfigure it.

This is what actually happens when the user formats the file in the IDE with Prettier integration. IDE formats the file in 3 steps:

• format the whole file (using Prettier + configured Prettier plugins) -- all is fine at this stage
• optimize imports (using IDE's own engine) -- so far so good, no changes in file compared to the previous step
• format imports block (using Prettier + configured Prettier plugins) -- and here the problem appears

So no functionality will be lost if the prettier-plugin-organize-imports plugin does nothing on the 3rd step

from prettier-plugin-organize-imports.

Or do you mean the plugin should only do nothing if a range is provided? Could make sense

I agree with this too. I think the plugin should just do nothing if a range is passed.

from prettier-plugin-organize-imports.

Alright, makes sense. Or at least automatically force-enabling noDestructiveCodeActions if only a range is formatted, since in a partial format it's kinda guaranteed that it would misbehave.

Anyway, problem: it seems that rangeStart and rangeEnd are always set, even when formatting the whole file 😕

@alexander-doroshko just wondering, why is the order not just

• optimize imports using IDE's own engine
• format the whole file using Prettier

Makes a lot more sense to me and prevents the issue in the first place, plus you would be faster because don't double-format a part of the document.

from prettier-plugin-organize-imports.

just wondering, why is the order not just...

The current implementation was just the easiest and the most logical way to add Prettier integration to the IDE. The IDE has its own smartness - its own code formatting and import optimization tools. So, it has its order of actions. The easiest way to add Prettier integration was to catch the moments when the IDE wanted to format the whole file or a range and to call Prettier for this file or this range. This way we've got such an order of actions. Any other solution would require significant changes in the IDE engine.

from prettier-plugin-organize-imports.

Anyway, problem: it seems that rangeStart and rangeEnd are always set, even when formatting the whole file 😕

Do you have an idea how to figure out if the plugin received a range or the full file, given that it seems to always set rangeEnd?

from prettier-plugin-organize-imports.

Do you have an idea how to figure out if the plugin received a range or the full file

Sorry, no idea, I've never been writing a prettier plugin

from prettier-plugin-organize-imports.

Anyway, problem: it seems that rangeStart and rangeEnd are always set, even when formatting the whole file 😕

Do you have an idea how to figure out if the plugin received a range or the full file, given that it seems to always set rangeEnd?

@simonhaenisch

As the code is just a string and the rangeEnd is just the length of the string, isn't it enough to just check if the rangeEnd !== code.length?

I just did some very quick testing of this and it seems to solve the issue

const organizeImports = (code, options) => {
	if (
		code.includes('// organize-imports-ignore') ||
		code.includes('// tslint:disable:ordered-imports') ||
		code.length !== options.rangeEnd ||
		options.rangeStart !== 0
	) {
		return code;
	}

from prettier-plugin-organize-imports.

I can try to add some tests and submit a PR tonight or feel free to take that code if it works

from prettier-plugin-organize-imports.

@simonhaenisch This should solve the issue with rangeEnd when using the CLI #112

@alexander-doroshko This solves the rangeEnd issue via the CLI but doesn't seem to solve the IDE issue.
When testing the original example from this issue in WebStorm, I don't see the imports disappear when running a reformat, with or without my fix.

When I use some slightly longer code, I see the issue in WebStorm with and without my fix.
Is WebStorm caching the node_modules?
Or doing something else?

import { compare } from 'fast-json-patch';

const x = 100;

function y() {
  compare({}, {});
}

from prettier-plugin-organize-imports.

Close/open the project in the IDE to make sure there are no caching issues

from prettier-plugin-organize-imports.

Hey thanks for the PR, will try to get to it soon!

Kinda weird though that code.length would be larger than rangeEnd, I was assuming that it would actually only pass the code of the affected range to the plugin 🤔 because if code contains the whole file, then doing a partial format shouldn't actually remove seemingly unused imports?

from prettier-plugin-organize-imports.

That's an interesting point actually.
From what I could tell, the organizeImports method seemed to be called twice.
Once with the full code and then again just with the range.
I can't see this in prettier docs anywhere.

I guess this could be a way to gather the context of the whole file and then just format a range.
However, I would suspect most people using this plugin aren't using rangeEnd and it's just suitable to do nothing destructive if it is used.

from prettier-plugin-organize-imports.

Nice work @simonhaenisch!
I can confirm this fixes the bug in the IDE too 🎉

from prettier-plugin-organize-imports.