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

order/newline-after-import: Add support for TypeScript's "export import" #1830

Conversation

be5invis
Copy link
Contributor

@be5invis be5invis commented Jun 19, 2020

This PR adds support for TypeScript's "export import", typically used to re-export things.
In current eslint-plugin-import they are treated as imports rather than exports.
Related: #1829.
cc. @ljharb

…ort object"

Co-authored-by: be5invis <[email protected]>
Co-authored-by: Manuel Thalmann <[email protected]>
@coveralls
Copy link

coveralls commented Jun 19, 2020

Coverage Status

Coverage increased (+0.004%) to 97.895% when pulling 2962628 on be5invis:be5invis-add-test-for-export-import-typescript into 4a38ef4 on benmosher:master.

@ljharb ljharb force-pushed the be5invis-add-test-for-export-import-typescript branch from 354e6f0 to 9844da9 Compare June 19, 2020 06:21
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

@ljharb ljharb force-pushed the be5invis-add-test-for-export-import-typescript branch 2 times, most recently from 587f61f to dda23cc Compare June 19, 2020 06:46
@ljharb ljharb changed the title Add unit test for TypeScript's "export import" [Tests] order/newline-after-import: Add unit tests for TypeScript's "export import" Jun 19, 2020
@be5invis
Copy link
Contributor Author

@ljharb Hah, these tests reveal a bug in newline-after-import. I will file another issue.

@ljharb
Copy link
Member

ljharb commented Jun 19, 2020

lol, thanks :-) we can just turn this PR into the fix for it.

@be5invis
Copy link
Contributor Author

Add @manuth

@ljharb I may not able to create a fix because I cannot get eslint-plugin-import's testing work on my machine, it always ends with these errors:

npm run test

> [email protected] pretest Z:\eslint-plugin-import
> linklocal

tests\files\order-redirect
resolvers\webpack
tests\files\symlinked-module
tests\files\order-redirect-scoped
utils
resolvers\node

Linked 6 dependencies

> [email protected] test Z:\eslint-plugin-import
> npm run tests-only


> [email protected] tests-only Z:\eslint-plugin-import
> npm run mocha tests/src


> [email protected] mocha Z:\eslint-plugin-import
> cross-env BABEL_ENV=test nyc -s mocha "tests/src"

internal/modules/cjs/loader.js:985
  throw err;
  ^

Error: Cannot find module 'Z:\eslint-plugin-import\node'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:982:15)
    at Function.Module._load (internal/modules/cjs/loader.js:864:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}
events.js:288
      throw er; // Unhandled 'error' event
      ^

Error: spawn nyc ENOENT
    at notFoundError (Z:\eslint-plugin-import\node_modules\cross-spawn\lib\enoent.js:11:11)
    at verifyENOENT (Z:\eslint-plugin-import\node_modules\cross-spawn\lib\enoent.js:46:16)
    at ChildProcess.cp.emit (Z:\eslint-plugin-import\node_modules\cross-spawn\lib\enoent.js:33:19)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:275:12)
Emitted 'error' event on ChildProcess instance at:
    at ChildProcess.cp.emit (Z:\eslint-plugin-import\node_modules\cross-spawn\lib\enoent.js:36:37)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:275:12) {
  errno: 'ENOENT',
  code: 'ENOENT',
  syscall: 'spawn nyc'
}

@manuth
Copy link
Contributor

manuth commented Jun 19, 2020

@be5invis for getting this repo to work on a windows machine I recommend you to use "Windows Subsystem for Linux" aka. WSL (VSCode provides extensions for developing projects inside WSL.

If you don't want to use WSL, you can upgrade the nyc dependency:

npm install --no-save nyc@latest

After this, tests should work fine on windows.

@manuth
Copy link
Contributor

manuth commented Jun 19, 2020

@be5invis do I understand correctly, that your goal is, that export import statements basically are ignored by the import/order rule?

@be5invis
Copy link
Contributor Author

@manuth
I think import/order ignores "object imports" so I added similar ignoring to import/newline-after-import.
Upgrading nyc fixed the test runner so I can now make a fix 🥂.

@manuth
Copy link
Contributor

manuth commented Jun 19, 2020

Recent changes in #1823 cause them to not be ignored by import/order 😅
Looks like there's more work to be done for supporting TSImportEqualsDeclaration...

@manuth
Copy link
Contributor

manuth commented Jun 19, 2020

Shall we discuss TSImportEqualsDeclarations right here or should I create a separate PR?

@be5invis
Copy link
Contributor Author

@manuth Ah I understand what you mean...
There's also a isExport property for TSImportEqualsDeclarations. We can use that to ignore export imports.

@manuth
Copy link
Contributor

manuth commented Jun 19, 2020

Exactly... it's not just this, sadly...
As we have learned, there are two different kinds of TSImportEqualsDeclarations: Object-literal imports and module-imports.
Each of them can either be imported or imported and exported both at the same time:

// Modules
import fs = require("fs");
// or
export import fs = require("fs");

import { ESLint } from "eslint";
// Object-literals
import LintResult = ESLint.LintResult;
// or
export import LintResult = ESLint.LintResult;

Problems About Exports

As you have brought up in issue #1829, export imports can occur anywhere (that is in namespaces, in nested contexts etc.).
Imo, this can only be solved by letting import/order ignore export imports.

Problems About Object-Literals

My last changes in mentioned PR caused object-imports to be forced to be alphabetized as well.
I've noticed, that this is a bad behavior, too, as object-imports might depend on each other:

import A = ESLint;
import LintResult = A.LintResult; // Reports an error because "ESLint" > "A.LintResult"

I don't really know what to do about it... either we could set the name of each object-import to "", which causes any order of object-imports to be allowed.
Otherwise we could just let import/order ignore object-imports as well.

Also there's more to be done about TSImportEqualsDeclarations. I think we should add support for this sort of import-expressions for all the other rules as well.

@be5invis
Copy link
Contributor Author

@manuth Agree. We could file other work items for further support of these.

@manuth
Copy link
Contributor

manuth commented Jun 19, 2020

Alright! What about fixing object-imports and export imports?
Shall we just ignore them in the import/order-rule?

Copy link
Contributor

@manuth manuth left a comment

Choose a reason for hiding this comment

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

I've added some comments here and there where there are some changes to be applied.

src/rules/order.js Show resolved Hide resolved
src/rules/newline-after-import.js Outdated Show resolved Hide resolved
@be5invis be5invis changed the title [Tests] order/newline-after-import: Add unit tests for TypeScript's "export import" order/newline-after-import: Add support for TypeScript's "export import" Jun 19, 2020
@ljharb ljharb force-pushed the be5invis-add-test-for-export-import-typescript branch from 781937c to 2962628 Compare June 19, 2020 16:57
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.

4 participants