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

Fixed flow types imports #1106

Merged
merged 3 commits into from
Jun 30, 2018
Merged

Fixed flow types imports #1106

merged 3 commits into from
Jun 30, 2018

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

@@ -30,6 +30,9 @@ module.exports = {
node.specifiers.forEach(function (im) {
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
6 participants