Tslint: fix on grouped-import is deleting export statement

[jonyeezs]

Bug Report

• __TSLint version__: 5.13.1
• __TypeScript version__: 3.1.6
• __Running TSLint via__: (pick one) CLI - npx tslint -p tsconfig.json

TypeScript code being linted

import { version } from '../../package.json';

// This file can be replaced during build by using the `fileReplacements` array.
// `ng build --prod` replaces `environment.ts` with `environment.prod.ts`.
// The list of file replacements can be found in `angular.json`.

export const environment = {
  // Default Angular configuration.
  production: false,

  // Custom app configuration.
  Version: version,
  ApiUrl: 'https://url',
  IdentityUrl: 'https://url',
  MarketingUrl: 'https://url',
  ClientId: 'webapp',
  LogentriesToken: '',
  MixpanelToken: '',
  IntercomAppId: '',
  LogToConsole: true,
};

import 'zone.js/dist/zone-error'; // Included with Angular CLI.

with tslint.json configuration:

{
  "rulesDirectory": ["codelyzer"],
  "rules": {
    "max-length": false /* Let prettier handle it */,
    "arrow-return-shorthand": true,
    "callable-types": true,
    "class-name": true,
    "comment-format": [true, "check-space"],
    "curly": [true, "ignore-same-line"],
    "deprecation": {
      "severity": "warn"
    },
    "eofline": true,
    "forin": true,
    "import-blacklist": [true, "rxjs/Rx"],
    "import-spacing": true,
    "indent": [true, "spaces"],
    "interface-over-type-literal": true,
    "label-position": true,
    "member-access": false,
    "member-ordering": [
      true,
      {
        "order": ["static-field", "instance-field", "static-method", "instance-method"]
      }
    ],
    "no-arg": true,
    "no-bitwise": true,
    "no-console": [true, "debug", "info", "time", "timeEnd", "trace"],
    "no-construct": true,
    "no-debugger": true,
    "no-duplicate-super": true,
    "no-empty": false,
    "no-empty-interface": true,
    "no-eval": true,
    "no-inferrable-types": [true, "ignore-params"],
    "no-misused-new": true,
    "no-non-null-assertion": true,
    "no-redundant-jsdoc": true,
    "no-shadowed-variable": true,
    "no-string-literal": false,
    "no-string-throw": true,
    "no-switch-case-fall-through": true,
    "no-trailing-whitespace": true,
    "no-unnecessary-initializer": true,
    "no-unused-expression": true,
    "no-use-before-declare": true,
    "no-var-keyword": true,
    "object-literal-sort-keys": false,
    "one-line": [true, "check-open-brace", "check-catch", "check-else", "check-whitespace"],
    "ordered-imports": [
      true,
      {
        "grouped-imports": true
      }
    ],
    "prefer-const": true,
    "radix": true,
    "triple-equals": [true, "allow-null-check"],
    "typedef-whitespace": [
      true,
      {
        "call-signature": "nospace",
        "index-signature": "nospace",
        "parameter": "nospace",
        "property-declaration": "nospace",
        "variable-declaration": "nospace"
      }
    ],
    "unified-signatures": true,
    "variable-name": false,
    "whitespace": [true, "check-branch", "check-decl", "check-operator", "check-separator", "check-type"],
    "angular-whitespace": [true, "check-interpolation"],
    "use-view-encapsulation": true,
    "contextual-life-cycle": true,
    "banana-in-box": true,
    "no-output-named-after-standard-event": true,
    "no-output-on-prefix": true,
    "use-input-property-decorator": true,
    "use-output-property-decorator": true,
    "use-host-property-decorator": true,
    "no-input-rename": true,
    "no-output-rename": true,
    "use-life-cycle-interface": true,
    "use-pipe-transform-interface": true,
    "component-class-suffix": true,
    "directive-class-suffix": true,
    "component-selector": [true, "element", "ew", "kebab-case"]
  }
}

Actual behavior

Error on running tslint:

ERROR: ...environment.ts:25:1 - Imports from this module are not allowed in this group. The expected groups (in order) are: libraries, parent directories, current directo
ry.

That is expected. But...

It removed my export statement. This is what --fix did

import 'zone.js/dist/zone-error'; // Included with Angular CLI.

import { version } from '../../package.json';

Expected behavior

import 'zone.js/dist/zone-error'; // Included with Angular CLI.

import { version } from '../../package.json';

// This file can be replaced during build by using the `fileReplacements` array.
// `ng build --prod` replaces `environment.ts` with `environment.prod.ts`.
// The list of file replacements can be found in `angular.json`.

export const environment = {
  // Default Angular configuration.
  production: false,

  // Custom app configuration.
  Version: version,
  ApiUrl: 'https://url',
  IdentityUrl: 'https://url',
  MarketingUrl: 'https://url',
  ClientId: 'webapp',
  LogentriesToken: '',
  MixpanelToken: '',
  IntercomAppId: '',
  LogToConsole: true,
};

[RoystonS]

Note that it actually deletes _any code_ that isn't an import anywhere inside the imports area: Comments (which can wreck //tslint or //eslint excludes), export statements, let statements, anything.

[lukeautry]

Ran this on a big codebase and it deleted a ton of code.

What is the expected behavior here? tslint could:
1) Force the non-import code to a block below the import groups
2) Try to keep non-import code as close to its original location as possible

[adidahiya]

@lukeautry option 2 would be great, if anyone is considering tackling this bug.

// cc @abierbaum, in case he has seen this issue after submitting the initial implementation of this feature

[lukeautry]

@adidahiya See #4583 - not Solution 2 as described above, but I think it's a pretty non-invasive solution that stops the bleeding til a more robust solution can be worked on.

[lukeautry]

@JoshuaKGoldberg Any chance you can take a look at the PR for this?

[jeysal]

Noticed there was already a bug for this after creating a repro. In case it helps (and maybe to check if a fix also works in this case), here's a pretty minimal repro, instructions in readme:
https://github.com/jeysal/tslint-ordered-imports-repro

[lukeautry]

@jeysal There's already a fix merged in for this, but I don't think there's been a new release for the tslint npm package yet.

[abierbaum]

@lukeautry Thanks for the fix. Sorry the code didn't work as it should have.

@adidahiya I didn't see anything like this in my testing with our codebase, but now looking at it I remember thinking to myself that it seemed odd how the original code was doing the fixes. I probably slipped in a regression due to some confusion in that part of the code. I would love to see something that could do Option 2 above, but I don't see an easy way to do this off hand. :(

[jeysal]

@jeysal There's already a fix merged in for this, but I don't think there's been a new release for the tslint npm package yet.

Sorry, didn't take a look at the PR. Should perhaps close this then

[JoshuaKGoldberg]

Indeed, #4583 should have fixed this. Please open a new issue to restart discussions & version information if it pops up again after the next release!