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

v2.0.0 prefers '.d.ts' files to '.js' files when both exist. #33

Closed
jgerigmeyer opened this issue Oct 16, 2019 · 12 comments
Closed

v2.0.0 prefers '.d.ts' files to '.js' files when both exist. #33

jgerigmeyer opened this issue Oct 16, 2019 · 12 comments

Comments

@jgerigmeyer
Copy link

In v2.0.0, it seems that the change from:

  const extensions = Object.keys(require.extensions).concat(
    '.ts',
    '.tsx',
    '.d.ts',
  );

to:

const extensions = ['.ts', '.tsx', '.d.ts'].concat(
  // eslint-disable-next-line node/no-deprecated-api
  Object.keys(require.extensions),
  '.jsx',
)

...means that if a node module is imported and contains both .js and .d.ts files, the .d.ts file is now resolved instead of the .js one. This results in import/default errors, e.g.:

import React from 'react';
error: No default export found in module (import/default)

Maybe I'm missing something obvious?

@JounQin
Copy link
Collaborator

JounQin commented Oct 16, 2019

Yes, it's just working as expected.
See #31 (comment)

@alexgorbatchev
Copy link
Collaborator

@JounQin would it make sense to document this behavior somewhere in README?

@joaovieira
Copy link

joaovieira commented Oct 29, 2019

@JounQin Sorry, don't know what you mean by "it's working as expected". Both here and here. I'm having issues with lodash as well, just as in #31.

Screen Shot 2019-10-29 at 16 39 18

In all of these scenarios, there is a default export that always used to work (with esModuleInterop):

// node_modules/dataloader/index.d.ts
declare class DataLoader {}
export = DataLoader;
// node_modules/@types/lodash/snakeCase.d.ts
import { snakeCase } from "./index";
export = snakeCase;

So it's not working as we're expecting.

@JounQin
Copy link
Collaborator

JounQin commented Oct 29, 2019

It's not ES Module default export, it's commonjs, that why the eslint-plugin-import complains about no default export.

@JounQin
Copy link
Collaborator

JounQin commented Oct 29, 2019

And you should issue to eslint-plugin-import for help supporting that instead.

@joaovieira
Copy link

joaovieira commented Oct 29, 2019

With esModuleInterop.

On top of those already mentioned, import/order also started to raise:

Screen Shot 2019-10-29 at 17 28 05

express: external types in @types/express - seen as "internal"
http-status-codes: own types - seen as "external"

I'm not that well versed in eslint-plugin-import and/or eslint-import-resolver-typescript to understand what is going on - or even where the boundaries are. So can't say yet where the problem is, but will dig deeper. But, as a consumer, it's not working as expected.

Regarding disabling the rules: typescript-eslint/typescript-eslint#154 (comment)

@JounQin
Copy link
Collaborator

JounQin commented Oct 29, 2019

esModuleInterop is TypeScript spec and eslint-plugin-import doesn't understand it, that's why I told you to issue to it instead.

And also, eslint-plugin-import considers foo and @types/foo as different packages, so it's still as expected on eslint-import-resolver-typescript's side.

Please, issue to eslint-plugin-import instead if it's not as expected for you.

@joaovieira
Copy link

joaovieira commented Oct 30, 2019

Ok. For the import/order, I've raised import-js/eslint-plugin-import#1525 and fixed in import-js/eslint-plugin-import#1526.

For the import/default, where do you think the problem would be? In the parser or the import plugin?

In 1.1.1 reading from the JS source (lodash/snakeCase.js) works fine:

var snakeCase = createCompounder(function(result, word, index) {
  return result + (index ? '_' : '') + word.toLowerCase();
});

module.exports = snakeCase;

In 2.0.0 using the TS export syntax in the type definition (@types/lodash/snakeCase.d.ts) it doesn't work:

import { snakeCase } from "./index";
export = snakeCase;

@JounQin
Copy link
Collaborator

JounQin commented Oct 30, 2019

I've told you the type definition is commonjs format and eslint-plugin-import doesn't treat it as an ES Module then. It doesn't even know any compilerOptions about esModuleInterop.

And it's not regression in my opinion then, it's just the correct behavior. Of course, it could be better to support esModuleInterop for eslint-plugin-import, and there is nothing we can do on side of eslint-import-resolver-typescript.

@joaovieira
Copy link

joaovieira commented Oct 30, 2019

@JounQin I don't know how you can maintain this project with that attitude. I'm trying to help as you've seen. I'm not saying the problem is in the resolver - I already understood it isn't after having to dig in myself. But all these issues are related to the change in the way the resolver resolves files. You can't expect the rest of the ecosystem to automatically fix itself and just ignore it. As it stands, migrating to 2.0.0 does cause regressions for users.

I've already understood the issue, so I'll move away from this thread as its not helpful.

@JounQin
Copy link
Collaborator

JounQin commented Oct 30, 2019

It is very clear I've answered your latest question at my first comment. It's great to address an issue or even usage question for this project, but I can't understand why I should answer the same question for several times.

And also, I'm not ignoring them, I've answered every comment about this and pinned the most related two issues. If you'd like to help, you can also raise a PR mentioned about this.

@JounQin
Copy link
Collaborator

JounQin commented Oct 30, 2019

And also, we do have a CHANGELOG since v2.0.0, it's a BREAKING CHANGE, that's why we bump the major version.

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

No branches or pull requests

4 participants