Skip to content

Conversation

@syymza
Copy link
Contributor

@syymza syymza commented May 18, 2018

#1057 has fixed #921 for the basic case, in which an import is the only thing imported from a specific file.
Sometimes types and functions/classes need to be imported from the same file. This PR fixes also that specific instance:

import  { type MyOpaqueType, MyClass } from "./flowtypes"',

I have added some tests for all these cases

Fixes #1115.

@coveralls
Copy link

coveralls commented May 18, 2018

Coverage Status

Coverage increased (+2.8%) to 97.268% when pulling 1d9ae51 on syymza:flow-types-fix into ebafcbf on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (im.type !== type) return

// ignore type imports
if(im.importKind === 'type') return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after if?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Copy link
Member

@benmosher benmosher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a test that

import  { type MyOpaqueType, MyMissingClass } from "./flowtypes"

reports for only MyMissingClass?

@benmosher
Copy link
Member

is this importKind test from line 15 needed anymore, after your change?

if (node.source == null || node.importKind === 'type') return

https://github.com/benmosher/eslint-plugin-import/pull/1106/files#diff-ff17d5a6c266bff5df3b40f4f70afc5aR15

if not, would like to get it out of there as your change seems more targeted and sufficient on its own.

@syymza
Copy link
Contributor Author

syymza commented May 18, 2018

@benmosher Added the requested test.
node.importKind === 'type' checks the imports having the type word outside of the {} specs. For this reason it cannot be removed, otherwise the other tests I have added in this PR will fail

@syymza
Copy link
Contributor Author

syymza commented May 28, 2018

🙄

@fizwidget
Copy link

Hey @benmosher, could you take another look at this one when you get a chance? Would love to see it merged 🙂

@ljharb ljharb dismissed benmosher’s stale review June 30, 2018 15:18

tests added

@ljharb ljharb merged commit 37554fe into import-js:master Jun 30, 2018
@bstuff
Copy link

bstuff commented Jul 27, 2018

Does this work when there are comments in imported module like:
foo.js:

/* flow-include
  export type Foo = string | number;
*/

bar.js:

import { type Foo } from './foo';

?

@ljharb
Copy link
Member

ljharb commented Jul 27, 2018

@bstuff not sure; can you file a new issue if it doesn’t work?

@syymza
Copy link
Contributor Author

syymza commented Jul 28, 2018

@bstuff it should :)

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

Development

Successfully merging this pull request may close these issues.

import/named doesn't ignore Flow identifier syntax type import import/named doesn't detect Flow Opaque types in flow comments

6 participants