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

Support TypeScript import = require syntax #1244

Closed
j-f1 opened this issue Dec 15, 2018 · 16 comments · Fixed by #1785
Closed

Support TypeScript import = require syntax #1244

j-f1 opened this issue Dec 15, 2018 · 16 comments · Fixed by #1785

Comments

@j-f1
Copy link
Contributor

j-f1 commented Dec 15, 2018

Currently this project only parses regular import statements as import statements. However, TypeScript also supports another syntax:

// TSImportEqualsDeclaration
import foo = require('bar')
//           ^^^^^^^^^^^^^^
// TSExternalModuleReference

Would it be possible for the plugin to check these imports as well?

@ljharb
Copy link
Member

ljharb commented Dec 15, 2018

TypeScript's module system is broken unless synthetic imports and esModuleInterop are enabled. With these settings, I don't believe there's any use case for the import = require or export = syntax. Can you confirm?

@j-f1
Copy link
Contributor Author

j-f1 commented Dec 15, 2018

In what way is TS’s module system broken without those options? I’ve never heard that before.

@ljharb
Copy link
Member

ljharb commented Dec 15, 2018

For example, a common thing people do without those options is import * as React from ‘react’, which is incorrect - React only has a default export, and import * brings in a frozen Module Namespace object, which TS does not by default.

@j-f1
Copy link
Contributor Author

j-f1 commented Dec 15, 2018

Ah, interesting.

Still, since people still use TypeScript without esModuleInterop and the syntax is valid, there doesn’t seem to be any harm in adding support for it.

@ljharb
Copy link
Member

ljharb commented Dec 15, 2018

I suppose that’s fair too.

@bradzacher
Copy link
Contributor

I wouldn't agree that typescript's module system is broken in terms of react.

Whilst yes, react is built as a es6 module, and internally it has a default export of the React object [1], the package itself uses commonjs module.exports for the base export [2].

Which means that using React with synthetic default imports is technically wrong.

[1] https://github.com/facebook/react/blob/master/packages/react/src/React.js#L109
[2] https://github.com/facebook/react/blob/master/packages/react/index.js#L16

@ljharb
Copy link
Member

ljharb commented Dec 16, 2018

@bradzacher React is a CJS module - which means it only has a default export. https://github.com/facebook/react/blob/master/packages/react/index.js#L16 ensures that the module.exports values is React itself - so import * as React from 'react' is never correct; although I agree that import { Component } from 'react' is very questionable.

@bradzacher
Copy link
Contributor

Ultimately it shows a weakness of using CJS to represent es6 modules.

const exp = { a, b, c };
module.exports = exp;

// does this mean
export default exp;
// or
export { a, b, c };

The intent is definitely the former, but based on how the transpilation works, reality is the latter.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2018

… which means the intent is "correct" and the transpiler (tsc, in this case, with default settings) is broken. With a few settings (which I believe are synthetic imports and ES module interop) it behaves properly.

(I wouldn't classify this as a weakness, more as, CJS only has default exports; ESM adds named exports. So, lacking a standard interop approach, the most "correct" thing is to treat a CJS module as having a single default export)

RomainMuller added a commit to aws/jsii that referenced this issue Dec 16, 2019
Per import-js/eslint-plugin-import#1244, the `eslint/import` plug-in does not currently support the `import X = require(Y);` syntax, causing extraneous imports in this form to be ignored by the linter.
RomainMuller added a commit to aws/jsii that referenced this issue Dec 16, 2019
Per import-js/eslint-plugin-import#1244, the `eslint/import` plug-in does not currently support the `import X = require(Y);` syntax, causing extraneous imports in this form to be ignored by the linter.
@manuth
Copy link
Contributor

manuth commented May 27, 2020

I'm also waiting for this plugin to suport import x = require("x"); expressions.
While it might be quite confusing what's exactly the difference between import * as x from "x"; (which is supported by this module) and import x = require("x"); (which is not supported by this plugin) I try to explain it very briefly.

The module assert, for example, directly exports a method which is made for asserting truthy values:

const assert = require("assert");
assert(true); // does not throw
assert(false); //does throw

And here comes the clue:
I've found a quite good explanation on StackOverflow which might help you understanding.
While this here is perfectly valid typescript-code:
index.ts

import assert = require("./MyAssert");
assert(true);

This here is not:

import * as assert from "./MyAssert";
import * as MyClass from "./MyClass";

assert(true);
new MyClass();

Because while import x = require("x"); assigns the exported object to x, import * as x from "x" fetches all exported members and assigns them to x.

So there certainly is a reason why import [...] = require[...] exists and it'd be very awesome if this plugin would support said sort of import.

Thanks for this great plugin ^^

@ljharb
Copy link
Member

ljharb commented May 27, 2020

@manuth that's not quite correct; if you use esModuleInterop and synthetic imports (as tsc --init enables), then you never need import * as in TS (which is only useful in regular JS for laziness or metaprogramming), nor import = (which is nonstandard syntax). The solution is to fix your TS config so TS's module system isn't broken.

@manuth
Copy link
Contributor

manuth commented May 27, 2020

It is valid typescript code and therefore it would be quite nice if this plugin could handle it. That's all people commenting here were asking for.

@ljharb
Copy link
Member

ljharb commented May 28, 2020

I totally agree, which is why i put the "help wanted" label on this issue. I'm saying that regardless, the best solution is to avoid using this syntax in the first place, and I wanted to correct your explanation about it.

@manuth
Copy link
Contributor

manuth commented May 28, 2020

Thanks for your help then 😄
I'll try to get some PR done to get TypeScript's "import equals"-references to work.

@ExE-Boss
Copy link

ExE-Boss commented Oct 3, 2020

Note that import/first now has conflicts with import/order when import from and TypeScript’s import = are both used:

import { foo } from "./bar.js";
import path = require("path");
//~~~~~~~~~~~~~~~~~~~~~~~~~~~~ import/order: `path` import should occur before import of `./bar.js`
import path = require("path");
import { foo } from "./bar.js";
//~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ import/first: Import in body of module; reorder to top.

@ljharb
Copy link
Member

ljharb commented Oct 3, 2020

@ExE-Boss thanks for the report! a PR with a fix, or a PR with a failing test case, or an issue would be great (in that order ;-) ) :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants