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

TypeError: Cannot read property 'lastIndexOf' of undefined after upgrading from 2.23.4 to 2.24.1 #2199

Closed
mertia-himanshu opened this issue Aug 20, 2021 · 26 comments

Comments

@mertia-himanshu
Copy link

Steps to Reproduce
npm install
npm run fix:eslint

Repo link - https://github.com/tmtsoftware/esw-ocs-eng-ui/tree/eslint-plugin-import-issue
Branch - eslint-plugin-import-issue

TypeError: Cannot read property 'lastIndexOf' of undefined
Occurred while linting /Users/himanshu/projects/tmt/esw-ocs-eng-ui/test/utils/test-utils.tsx:1
    at removeQuerystring (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-import-resolver-typescript/lib/cjs.js:124:31)
    at Object.resolve (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-import-resolver-typescript/lib/cjs.js:51:14)
    at v2 (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-module-utils/resolve.js:117:23)
    at withResolver (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-module-utils/resolve.js:122:14)
    at fullResolve (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-module-utils/resolve.js:139:22)
    at Function.relative (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-module-utils/resolve.js:84:10)
    at remotePath (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-plugin-import/lib/ExportMap.js:743:379)
    at resolveImport (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-plugin-import/lib/ExportMap.js:743:460)
    at Object.getImport (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-plugin-import/lib/ExportMap.js:745:107)
    at ExportMap.get (/Users/himanshu/projects/tmt/esw-ocs-eng-ui/node_modules/eslint-plugin-import/lib/ExportMap.js:710:667)
ERROR: "eslint:check" exited with 2.```
@mmakarin
Copy link

Same here

@ljharb
Copy link
Member

ljharb commented Aug 20, 2021

That repo unfortunately doesn't reproduce the issue for me (with npm run eslint:check either).

@mmakarin do you have a repro repo?

@ljharb
Copy link
Member

ljharb commented Aug 20, 2021

Also, your stack trace seems like the error is coming from eslint-import-resolver-typescript - which is a separate project from this one. Can you file it there?

I'll be happy to reopen this if I can reproduce it and if the error is coming from projects in this repo.

@ljharb ljharb closed this as completed Aug 20, 2021
@jjangga0214
Copy link

jjangga0214 commented Aug 22, 2021

@ljharb This happens to me as well.
Maybe I can provide a repro later, but I want you to know that this may not be misreported.

@ljharb
Copy link
Member

ljharb commented Aug 22, 2021

@jjangga0214 i believe it’s happening, but if it’s an error in eslint-import-resolver-typescript, that’s not a package i maintain and isn’t part of this repo.

jjangga0214 added a commit to jjangga0214/hasura-schema-stitching-boilerplate that referenced this issue Aug 22, 2021
@NickHeiner
Copy link

I also got this, only after updating to 2.24.1. I'm trying to create a repro now.

@NickHeiner
Copy link

Partial info: This triggers on a line like:

source.ts:

import {a} from './b';

where b.ts is:

export * as b from './c';

And c.ts can be empty.

However, I recreated this precise setup in a standalone repo, and couldn't repro. So there's a necessary element I'm missing.

Separately, when inspecting the eslint-plugin-import source code, I found that if I insert a console.log in lib/ExportMap.js:

function processSpecifier(s, n, m) {var nsource = n.source && n.source.value; console.log({n}); 

I see the following output:

{
  n: {
    type: 'Identifier',
    name: 'clConstants',
    range: [ 647, 658 ],
    loc: { start: [Object], end: [Object] }
  }
}

And it does not appear that resolveImport, in the processSpecifier block, handles n.source being undefined.

@NickHeiner
Copy link

When I add that same console line in the repo where the issue doesn't repro, the line isn't hit at all. So it appears that processSpecifier isn't being called at all.

Does that help any figuring out what's going on? Ha.

@ljharb
Copy link
Member

ljharb commented Aug 23, 2021

It might be the case that resolvers weren't previously passed undefined, and now we are doing that, so even though the error is coming from the typescript resolver (and even though they could handle it), we can probably fix the problem a bit differently.

I'll try to create a failing test case; if I can, then we can solve it (but either way, it would be ideal to file a bug on the typescript resolver's repo)

@ljharb ljharb reopened this Aug 23, 2021
@NickHeiner
Copy link

Here's the output of yarn list --pattern eslint in a repo where I can repro this, and one where I can't:

Issue is present:

yarn list v1.22.10
├─ @typescript-eslint/[email protected]
├─ @typescript-eslint/[email protected]
├─ @typescript-eslint/[email protected]
├─ @typescript-eslint/[email protected]
│  ├─ @typescript-eslint/[email protected]
│  └─ [email protected]
├─ @typescript-eslint/[email protected]
├─ @typescript-eslint/[email protected]
│  ├─ @typescript-eslint/[email protected]
│  └─ [email protected]
├─ @typescript-eslint/[email protected]
│  ├─ @typescript-eslint/[email protected]
│  └─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
│  ├─ @typescript-eslint/[email protected]
│  ├─ @typescript-eslint/[email protected]
│  ├─ @typescript-eslint/[email protected]
│  └─ @typescript-eslint/[email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
│  └─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
│  └─ [email protected]
├─ [email protected]
└─ [email protected]
   └─ [email protected]

Issue is not present:

yarn list v1.22.5
├─ @typescript-eslint/[email protected]
├─ @typescript-eslint/[email protected]
├─ @typescript-eslint/[email protected]
├─ @typescript-eslint/[email protected]
├─ @typescript-eslint/[email protected]
│  └─ @typescript-eslint/[email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
├─ [email protected]
└─ [email protected]
Done in 0.17s.

Here's my unsuccessful attempt to repro this in a separate repo.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2021

ah - looks like we have a bunch of test failures (24) when I install eslint-import-resolver-typescript v2 locally - we only test with v1 atm. None of those failures, however, are this crash :-/ (many are `parseForESLint` from parser `@typescript-eslint/parser` is invalid and will just be ignored) We're also testing with v3 of @typescript-eslint/parser and apparently v4 is out. I filed #2203 to track that.

In the meantime, I could certainly avoid calling resolveImport with an undefined source, but without a regression test, that seems unwise :-/

@NickHeiner can you confirm that if you edit those two lines doing resolveImport(nsource) to resolveImport(nsource || s.source), it fixes the problem?

@NickHeiner
Copy link

That doesn't quite work. s is, for example:

{
    type: 'ExportAllDeclaration',
    source: {
      type: 'Literal',
      value: './constants',
      raw: "'./constants'",
      range: [Array],
      loc: [Object]
    },
    exportKind: 'value',
    exported: {
      type: 'Identifier',
      name: 'clConstants',
      range: [Array],
      loc: [Object]
    },
    range: [ 635, 678 ],
    loc: { start: [Object], end: [Object] }
  }

Whereas I think resolveImport expects a string. This results in stack trace:

TypeError: id.lastIndexOf is not a function
Occurred while linting /Users/nheiner/code/tvui/packages/darwin/src/components/PlaymodeLanguageDiscoveryControls/logger.ts:5
    at removeQuerystring (/Users/nheiner/code/tvui/node_modules/eslint-import-resolver-typescript/lib/cjs.js:125:31)
    at Object.resolve (/Users/nheiner/code/tvui/node_modules/eslint-import-resolver-typescript/lib/cjs.js:52:14)
    at v2 (/Users/nheiner/code/tvui/node_modules/eslint-module-utils/resolve.js:119:23)
    at withResolver (/Users/nheiner/code/tvui/node_modules/eslint-module-utils/resolve.js:124:14)
    at fullResolve (/Users/nheiner/code/tvui/node_modules/eslint-module-utils/resolve.js:141:22)
    at Function.relative (/Users/nheiner/code/tvui/node_modules/eslint-module-utils/resolve.js:85:10)
    at remotePath (/Users/nheiner/code/tvui/node_modules/eslint-plugin-import/lib/ExportMap.js:741:379)
    at resolveImport (/Users/nheiner/code/tvui/node_modules/eslint-plugin-import/lib/ExportMap.js:741:460)
    at Object.getImport (/Users/nheiner/code/tvui/node_modules/eslint-plugin-import/lib/ExportMap.js:743:134)
    at ExportMap.get (/Users/nheiner/code/tvui/node_modules/eslint-plugin-import/lib/ExportMap.js:708:667)

If I do s.source.value, then there is no crash. However, if I then change the import to import an invalid name, I don't get a lint error.

import {doesNotExist} from './b';

@ljharb
Copy link
Member

ljharb commented Aug 24, 2021

what is s.source.value in the latter instance?

@NickHeiner
Copy link

I assume it would still be './b', since I hadn't changed that part of the expression.

ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Aug 24, 2021
@Nantris
Copy link

Nantris commented Aug 24, 2021

Upgrading to 2.24.1 caused this issue for me. Going back to 2.24.0 resolved it.

@NickHeiner
Copy link

NickHeiner commented Aug 24, 2021

Seems like a bunch of people are getting this – can someone other than me try to make a repro (since I failed at it)? Or at least share what pattern it's happening on? Or at least say whether what I pasted above matches what you're seeing? 😄

@Nantris
Copy link

Nantris commented Aug 24, 2021

I was already downgrading and had lost the exact error message, but the callstack had the Webpack ESLint Resolver in it, so I think that whatever the issue is is not related to any specific resolver, although it certainly seems there's some interplay here.

I'll try to remember, at the end of my day, to reinstall 2.24.1 re-lint our project, and report back with the message.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2021

@NickHeiner @slapbox i'm going to cut a patch release shortly; if there's any way to try out the latest default branch of the plugin and confirm that it'll fix your problem, that would be most helpful.

@NickHeiner
Copy link

You mean you're going to publish a pre-release version to npm, right? Yes, I'm happy to test that.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2021

No, I'm going to publish the real one :-) i was just hoping to confirm from git-based testing in the meantime.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2021

v2.24.2 is released; please give it a shot.

@NickHeiner
Copy link

I tried it. I no longer get the crash, but when I change the import to be an invalid name, I no longer get a lint error. (I do get a TS error.)

import { invalidName } from 'log4tvui';

@Nantris
Copy link

Nantris commented Aug 25, 2021

No crashes with v2.24.2!

Thank you @ljharb!

@NickHeiner
Copy link

Should I open a new issue to track the error where import/named isn't firing for an invalid import?

@Nantris
Copy link

Nantris commented Aug 25, 2021

@NickHeiner I would say yes.

@ljharb
Copy link
Member

ljharb commented Aug 25, 2021

Yes, that would be great, thanks.

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

No branches or pull requests

6 participants