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

Update: Add support for flow type import/export declarations #7708

Closed

Conversation

zackargyle
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[X] Other, please explain:

  • This is kind of a bug fix, but not really.

What changes did you make? (Give an overview)

A lot of large companies are using FlowType now, which allows you to import/export type definitions via a type keyword in the declaration.

import type { FooType } from 'path/to/thing';
export type BarType = {
    type: string,
    foos: Array<FooType>,
};

The issue is that these type imports cannot be consolidated into a single import declaration, which means that if you want some function in a file, and a type in a file, it is required that you have an import declaration for each. This breaks the no-duplicate-imports rule. We want the following example to not error.

import Foo from 'path/to/foo';
import type { FooType } from 'path/to/foo';

The changes shouldn't be noticeable to people not using Flow, but for those who are they will now be able to use this rule without issue.

Is there anything you'd like reviewers to focus on?

As far as I could tell there was no good way to add a plugin to a test suite, so I couldn't actually add any new tests for the Flow syntax since it requires the flow parser. The current tests still pass, and I verified the code works in our large Pinterest webapp codebase.

@mention-bot
Copy link

@zackargyle, thanks for your PR! By analyzing the history of the files in this pull request, we identified @SimenB, @vitorbal and @gyandeeps to be potential reviewers.

@jsf-clabot
Copy link

jsf-clabot commented Dec 7, 2016

CLA assistant check
All committers have signed the CLA.

@eslintbot
Copy link

Thanks for the pull request, @zackargyle! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@eslintbot
Copy link

Thanks for the pull request, @zackargyle! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@not-an-aardvark
Copy link
Member

Thanks for the pull request. However, we generally don't accept changes to ESLint's core rules that pertain to non-standard syntax extensions. To get around the issue with flow type imports/exports, I would recommend using the no-duplicates rule from eslint-plugin-import instead, which seems to be designed to address this same issue.

@zackargyle
Copy link
Contributor Author

Hey @not-an-aardvark. I wasn't aware of no-duplicates! Looks like it's doing the same thing I'm doing here except it doesn't include exports. Any reason we'd allow the non-standard extension for that rule and not this one? That said, I totally get the idea and I can work with AirBnB to update their config to only use no-duplicates if you're really against this PR.

@not-an-aardvark
Copy link
Member

Thanks for understanding. To clarify, no-duplicates isn't actually an ESLint core rule; it's part of an ESLint plugin and is not maintained by the ESLint team. The maintainers of that rule have decided to support flow type syntax, but ESLint generally doesn't do this for core rules, because there are a lot of nonstandard syntax extensions and it would be difficult for a rule to be consistent with all of them. (By a "core rule" I mean a rule that is included in the ESLint package.)

I wasn't aware that the no-duplicates rule doesn't support exports. If you leave an issue on the eslint-plugin-import repo it's possible they would be willing to support this.

I totally get the idea and I can work with AirBnB to update their config to only use no-duplicates if you're really against this PR

I assume you mentioned AirBnB because you're using their ESLint config. If you don't end up getting the eslint-config-airbnb package changed upstream, you could also override that rule in your config file:

rules:
  no-duplicate-imports: off # disable the no-duplicate-imports rule
  import/no-duplicates: error # enable the no-duplicates rule from eslint-plugin-import
extends: airbnb

@zackargyle
Copy link
Contributor Author

Appreciate the swift response. You guys are doing fantastic work and we really appreciate it. Thanks for all you do!

@zackargyle zackargyle closed this Dec 7, 2016
@zackargyle zackargyle deleted the no-duplicate-imports-flow-support branch December 7, 2016 04:55
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants