Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Remove absolute first from import/first #61

Merged
merged 1 commit into from
May 21, 2018

Conversation

rickyp-uber
Copy link
Contributor

@rickyp-uber rickyp-uber commented May 7, 2018

Removing absolute-first field from import/first as the order of the imports does not matter. What matters is avoiding any weird knowledge issues when putting code between the imports since imports are hoisted.

This also unlocks a nice organizational feature for React components where you group your imports

// Components - External
...

// Components - Internal
import {Button} from '../button';

// Utiltiies
import * as moment from 'moment';

Removing `absolute-first` field from import/first as the order of the imports does not matter. What matters is avoiding any weird knowledge issues when putting code between the imports since imports are hoisted. This also unlocks a nice organizational feature for React components where you group your imports
```
// Components - External
...

// Components - Internal
import {Button} from '../button';

// Utiltiies
import * as moment from 'moment';
```
@rickyp-uber rickyp-uber changed the title Removed absolute first from import/first Remove absolute first from import/first May 7, 2018
Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, we will discuss within the team.

My thoughts: Even with this rule you still have grouping, just a different kind. You declare all of your node_module imports above, which is a good thing, and typically the first place people look for such imports.

For rules that don't really matter like this one, I would opt to err on the side of consistency.

@rickyp-uber
Copy link
Contributor Author

The grouping has a functional purpose though (putting code between two imports may not get executed in the order you expect). Forcing the absolute imports to be first doesn't actually gain you any benefit.

Further if the argument is consistency why not use import/sort instead which is far more consistent and actually would reduce merge conflicts as the order of the imports is always the same. That said, I would sort based on path ordering, not import specifier ordering personally as the line import itself wont jump around as much.

@KevinGrandon
Copy link
Contributor

Agree about import/order. Filed this issue to look into it: #62

@rickyp-uber
Copy link
Contributor Author

Seems that whichever way you go, looks like you're up for removing absolute from import/first so mind stamping this?

@KevinGrandon
Copy link
Contributor

Seems that whichever way you go, looks like you're up for removing absolute from import/first so mind stamping this?

We'll discuss this with the team. In the meantime you could use .eslintrc.js overrides. I would recommend against that, but it's up to you.

@KevinGrandon
Copy link
Contributor

We've decided as a team that we want to go with eslint:recommended as well as type checking, only keeping linting rules which help prevent bugs. Because this does not prevent bugs, we'll go ahead and accept this PR. Thanks for your contribution.

@KevinGrandon
Copy link
Contributor

!merge

@old-fusion-bot old-fusion-bot bot merged commit c025831 into fusionjs:master May 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants