#601: Fixed default order for the order rule

[jfmengels]

Fixes #601.
This fixes the default order to be the one described by the documentation, and they have been inconsistent ever since the rule was introduced.

Though it fixes the implementation to correspond to what the documentation says, this is a fix that can create new linting errors for users that use internal modules and have not overridden the order. In this case, I think that can be accepted as a fix/patch, and not a breaking change/major, but wanted to have some confirmation.

Fortunately, I think that the use of internal modules is relatively low, so it should not break anything for most people.

cc @ljharb who I know is very passionnate about respecting semver and might have more experience with this kind of situation.

[coveralls]

Coverage remained the same at 97.646% when pulling 4a39693 on fix-order-defaults into 5a6c38d on master.

[coveralls]

Coverage remained the same at 97.646% when pulling 4a39693 on fix-order-defaults into 5a6c38d on master.

[coveralls]

Coverage remained the same at 97.646% when pulling 4c8be30 on fix-order-defaults into 5a6c38d on master.

[ljharb]

I'm actually kind of confused how this plugin even determines "internal" - does it read the webpack config?

Based on the code in https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md#enforce-a-convention-in-module-import-order , the "internal" module there should be indistinguishable from an npm-installed module.

[jfmengels]

@ljharb It uses the resolver system (that @benmosher knows way better than me), which AFAIK is either configured by the user or reads your webpack config (or Babel config, as you can do that too using Babel). When nothing is configured, you'll never get internal but always external (== npm package).

Obviously, I also forgot to mention that we have the option of simply fixing the docs to reflect the implementation.

[coveralls]

Coverage remained the same at 94.893% when pulling d326f1b on fix-order-defaults into 404f506 on master.

[ljharb]

While I think that its likely that very few users will be affected by this change, I do think it constitutes a breaking change, since it's been the case the entire time, across majors.

[jfmengels]

Right. So maybe the way to go would be to simply change the documentation. I'm fine with that.

[ljharb]

Let's update the docs now; and then open a separate PR for the breaking change that can be queued up for v3.

[benmosher]

@jfmengels is the originally-documented order more ideal than the way that it has been implemented?

If not, does it make sense to just update the docs and never make the breaking change?

[jfmengels]

is the originally-documented order more ideal than the way that it has been implemented?

A bit, yes. They're pretty much the same, but there is support for internal modules, which only occur when you use Babel/Webpack to change the resolver logic.

I'm fine with changing the docs instead of the implementation, and I think that's what I'll do. Let's close this PR and I'll make another one to fix the docs instead. Sounds good?
EDIT: Made a PR #623

[benmosher]

Makes sense to me.

I think once all the other issues stemming from #479 are fixed, this might actually be necessary, but if it hasn't been much of a pain point so far, probably makes sense to leave alone for now.

[benmosher]

reopening for v3 milestone tracking, though feel free to close and open a new issue/PR or hack around on this one