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

Bad automated fix for no-duplicates when using Flow. #2691

Open
Tracked by #2716
julienw opened this issue Jan 23, 2023 · 8 comments
Open
Tracked by #2716

Bad automated fix for no-duplicates when using Flow. #2691

julienw opened this issue Jan 23, 2023 · 8 comments

Comments

@julienw
Copy link

julienw commented Jan 23, 2023

This could be related to #2475, but I'm not sure.
I got this error after upgrading, but not exactly the same one. I'm using Flow too.

I had some code like this:

import {
  withChartViewport,
  type WithChartViewport,
} from 'firefox-profiler/components/shared/chart/Viewport';
...
import type { Viewport } from 'firefox-profiler/components/shared/chart/Viewport';

After the change, the last import type was reported as an error and merged into the first one. This makes some sense, but this was merged incorrectly:

import {
  withChartViewport,
  type WithChartViewport,
  Viewport,
} from 'firefox-profiler/components/shared/chart/Viewport';

Notice that the word type is missing. Without it, Flow is erroring.
I could add it manually, the eslint wouldn't error then.

I simply have the rule configured like this:

    'import/no-duplicates': 'error',

So in short, in this case I think the error was right but the autofix wasn't.

Could be related to #2686 but I'm not sure because the error seems different.

@julienw
Copy link
Author

julienw commented Jan 23, 2023

This looks related to this discussion: https://github.com/import-js/eslint-plugin-import/pull/2475/files#r1019468217

@ljharb
Copy link
Member

ljharb commented Jan 24, 2023

Clearly a bug, yes, thank you. cc @snewcomer

@ljharb
Copy link
Member

ljharb commented Jan 24, 2023

Looks like it fails specifically when prefer-inline is not set (which doesn't work for flow anyways).

@ljharb
Copy link
Member

ljharb commented Jan 24, 2023

Here's the git diff for the failing test case:

       errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
     }),
 
+    test({
+      code: `
+        import {
+          withChartViewport,
+          type WithChartViewport,
+        } from 'firefox-profiler/components/shared/chart/Viewport';
+
+        import type { Viewport } from 'firefox-profiler/components/shared/chart/Viewport';
+      `,
+      output: `
+        import {
+          withChartViewport,
+          type WithChartViewport,
+          type Viewport,
+        } from 'firefox-profiler/components/shared/chart/Viewport';
+      `,
+      errors: [
+        '\'firefox-profiler/components/shared/chart/Viewport\' imported multiple times.',
+        '\'firefox-profiler/components/shared/chart/Viewport\' imported multiple times.',
+      ],
+      parser: parsers.BABEL_OLD,
+    }),
+
     test({

@snewcomer
Copy link
Contributor

@julienw Am I seeing that Flow supports inline types in your example? Is there another canonical name it goes by? I can't seem to find it.

@bradzacher
Copy link
Contributor

Flow supports inline specifier types

import {
  Foo,
  type Bar,
  typeof Baz,
} from 'module';

Note that unlike in TS where it's legal to import a type without the type qualifier (except under specific compiler flags), it is never legal to import types without a type qualifier in flow.

Fun fact: flow has had this syntax for many, many years! Whereas TS is a checker and a transpiler, flow is just a checker and transpilation was handled by babel - hence the syntax was created to make it easy for babel to elide the imports (which is why TS later adopted it).

@snewcomer
Copy link
Contributor

Wow. Great to know! Thank you.

So I have a solution to fix issues in ‘no-duplicates’. But before finishing off and submitting, I wanted to get consensus / this is bad / or just general thoughts on this.

#2697 (comment)

@julienw
Copy link
Author

julienw commented Jan 30, 2023

FWIW It's supported by Flow but I couldn't find the documentation about it so it would be difficult to know for a non-Flow user :)

I'm not opinionated about the solution as long as it produces valid code.

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

No branches or pull requests

4 participants