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

[patch] order/TypeScript: ignore ordering of object imports #1831

Merged
merged 1 commit into from
Jun 23, 2020
Merged

[patch] order/TypeScript: ignore ordering of object imports #1831

merged 1 commit into from
Jun 23, 2020

Conversation

manuth
Copy link
Contributor

@manuth manuth commented Jun 19, 2020

Forcing object-imports to be alphabetized might lead to issues as they might depend on each other.

Example:

import { ESLint } from "eslint";
// Object-imports
import A = ESLint;
import OutputFixes = A.outputFixes;

OutputFixes([]);

Currently, this piece of code will report an error because A.LintResult has a lower rank than ESLint.
Alphabetizing these imports would cause this code to fail:

import { ESLint } from "eslint";
// Object-imports
import OutputFixes = A.outputFixes;
import A = ESLint;

OutputFixes([]);
Cannot read property 'outputFixes' of undefined

I see three different approaches to fix this:

  • Sort object-imports with following conditions:
    1. Number of dots (e.g. ts comes before ts.protocol and ts.protocol comes before ts.protocol.Request)
    2. Import-Name
  • Allow any sorting by setting names of all object-imports to ''
  • Ignore object-imports

Though I'd like the first approach the most as it definitely would work and even would fix code like the one above, the second approach is ways easier to implement which is why I just went for it.

Remarks

The first approach won't work as, for example, this code still would cause an error:

import { ESLint } from "eslint";
import B = ESLint;
import A = B;

Therefore I'd recommend to go for the second approach as done in this PR.
Feedback is welcome - do you guys think it's possible to implement the first mentioned approach at some point?

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.9%) to 89.97% when pulling 2539870 on manuth:object-import-order into 4a38ef4 on benmosher:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-7.9%) to 89.97% when pulling 2539870 on manuth:object-import-order into 4a38ef4 on benmosher:master.

@coveralls
Copy link

coveralls commented Jun 19, 2020

Coverage Status

Coverage increased (+0.002%) to 97.855% when pulling c38b169 on manuth:object-import-order into bfc50b7 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.9%) to 89.97% when pulling 2539870 on manuth:object-import-order into 4a38ef4 on benmosher:master.

@manuth
Copy link
Contributor Author

manuth commented Jun 19, 2020

So I just realized that the first approach wouldn't work out as something like

import { ESLint } from "eslint";
import B = ESLint;
import A = B;

Still would cause issues.
Sticking up with the solution of this PR (which is naming all object-imports '') seems to be the best way to go.

tests/src/rules/order.js Outdated Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
src/rules/order.js Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
@manuth manuth requested a review from ljharb June 22, 2020 21:14
@ljharb ljharb changed the title Allow Any Order of Object-Imports [patch] order/TypeScript: ignore ordering of object imports Jun 23, 2020
@ljharb ljharb merged commit c38b169 into import-js:master Jun 23, 2020
@manuth manuth deleted the object-import-order branch June 23, 2020 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants