Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Expand ordered-imports to support import alias = a.b.c.d; #4591

Closed
itanex opened this issue Mar 20, 2019 · 9 comments
Closed

Expand ordered-imports to support import alias = a.b.c.d; #4591

itanex opened this issue Mar 20, 2019 · 9 comments

Comments

@itanex
Copy link

itanex commented Mar 20, 2019

Rule Suggestion

More of a clarification on Ordered Imports. How does it actually handle this? I could not find any details.

import x = required("source/a");
import y = x.n1.n2.n3.n4.y;
import z = x.n1.n2.n3.n4.z;

How are the aliasing imports here designed to sort by this rule? If it is not part of this rule, shouldn't it be since these are imports? Is there another rule that is handling this?

Is your rule for a general problem or is it specific to your development style?
Clarification mainly. But perhaps a suggestion to enhance coverage of Ordered Imports.

What does your suggested rule do?
Handle import aliasing.

import x = ns1.ns2.x;

Not

import * as x from "source";
import { x as y } from "source"

List several examples where your rule could be used

Additional context

@adidahiya
Copy link
Contributor

What kind of "handling" would you like to see? I doubt the rule does anything smart / special with those kinds of import aliases right now. Have you tried it?

@itanex
Copy link
Author

itanex commented Mar 20, 2019

The codebase I am starting to work with uses this pattern, I have not used it before. I will have to implement or help implement TSLint support on this new codebase. This came up in a discussion about how we should promote import statement types. So before corrupting a full TFVC branch with my changes I thought I would inquire about this.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jul 27, 2019

Looking at #4810, this would be a breaking change to allow the rule to also parse import = imports, right? That seems reasonable if so, provided it's not a huge amount of code change to support edge cases around similarly named imports.

Edit: we should also talk about how the desired sorting & grouping for these should work. How can the rule's configuration indicate where to put these? Using =.* seems a little hacky.

@JoshuaKGoldberg JoshuaKGoldberg changed the title [Clarification] - TSLint - Ordered Imports -> import alias = a.b.c.d; Expand ordered-imports to support import alias = a.b.c.d; Jul 27, 2019
@mihneadb
Copy link

@JoshuaKGoldberg Thanks for looking into this!
I view it more of a bug fix, since right now using the fixer on files with import aliasing yields invalid files (blocks of code are moved breaking class declarations / aliases are placed before their referenced imports).
Open for suggestions on this stuff. I don't have that much context around tslint - I'm just trying to fix an issue I encountered on a repo that uses it.

@JoshuaKGoldberg
Copy link
Contributor

Oh no, a bug! Would you mind filing a separate issue for the bug? We might be able to resolve that separately and more quickly than the discussion of supporting namespace imports.

@mihneadb
Copy link

Oh no, a bug! Would you mind filing a separate issue for the bug? We might be able to resolve that separately and more quickly than the discussion of supporting namespace imports.

Sure thing! I opened #4815, hope it helps!

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Oct 2, 2019

Removing the Type: Breaking Change label per #4811.

@JoshuaKGoldberg
Copy link
Contributor

@itanex after thinking about this a bit, I'm going to go ahead and close this issue. 😢 import alias = imports are no longer a recommended practice in TypeScript, so having explicit logic in this rule is a maintenance burden I don't think TSLint should take on given #4534. Our priority right now is deprecation and security fixes.

Thanks for bringing this up and participating in the discussion! A couple followups you could take on:

  • Release a forked custom version of the rule that includes the proposed change
  • Bring this issue up in typescript-eslint

@itanex
Copy link
Author

itanex commented Oct 2, 2019

@JoshuaKGoldberg True, this was made back in March before it was widely known that TS lint was approaching EOL.

Moving this over to ESLint is the best move, while import alias =... may not be a TS recommended practice, ESLint covers all ES variants now and aliasing is not shunned in some of those circles.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants